feat(api): expose balance history#1641
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Balance History API (GET /api/v1/balances, GET /api/v1/balances/{id}) with UUID validation, read-scope authorization, account/currency/date filtering, Pagy pagination, Jbuilder serialization (formatted amounts + minor-unit cents), NDJSON balance export, OpenAPI docs, and request/controller/exporter tests. ChangesBalance history API + NDJSON export
Sequence DiagramsequenceDiagram
participant Client
participant BalancesController
participant BaseController
participant BalanceModel
participant Database
rect rgba(100, 149, 237, 0.5)
Note over Client,Database: GET /api/v1/balances (Index)
Client->>BalancesController: GET /api/v1/balances?account_id=...¤cy=USD
BalancesController->>BaseController: ensure_read_scope
BaseController-->>BalancesController: authorization result
BalancesController->>BaseController: safe_page_param / parse_date_param
BaseController-->>BalancesController: validated params
BalancesController->>BalanceModel: balances_scope -> apply_filters -> order -> paginate
BalanceModel->>Database: SELECT ... WHERE ... ORDER BY date, created_at LIMIT...
Database-->>BalanceModel: rows + count
BalancesController->>BalancesController: render :index (Jbuilder)
BalancesController-->>Client: 200 JSON { balances: [...], pagination: {...} }
end
rect rgba(144, 238, 144, 0.5)
Note over Client,Database: GET /api/v1/balances/{id} (Show)
Client->>BalancesController: GET /api/v1/balances/{id}
BalancesController->>BaseController: validate UUID / ensure_read_scope
BaseController-->>BalancesController: validation & auth
BalancesController->>BalanceModel: balances_scope.find(id)
BalanceModel->>Database: SELECT ... WHERE id = ?
Database-->>BalanceModel: balance
BalancesController->>BalancesController: render :show (Jbuilder)
BalancesController-->>Client: 200 JSON { balance: {...} }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
brin-security-scanner
Bot
added
contributor:flagged
|
| Dimension | Score | What it measures |
|---|---|---|
| Identity | 30 | Account age, contribution history, GPG keys, org memberships |
| Behavior | 80 | PR patterns, unsolicited contribution ratio, activity cadence |
| Content | 100 | PR body substance, issue linkage, contribution quality |
| Graph | 30 | Cross-repo trust, co-contributor relationships |
Analyzed by Brin · Full profile
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Overview
Good addition — balance history is genuinely useful for backup/verification and the overall structure (scoped to accessible accounts, read-only, proper auth/scope checks, full OpenAPI+Minitest+rswag coverage) follows the project's conventions well. A few issues need addressing before merge.
Bugs
N+1 in data_exporter.rb (see inline) — The includes(:balances) is silently ignored because find_each on a scoped association issues a fresh query. Either drop the includes or flatten the query to a single Balance.joins(:account).where(...) pass, consistent with how the rest of the exporter works.
Potential 500 on GET /api/v1/balances/:id with non-UUID input (see inline) — AccountsController guards against this explicitly; BalancesController doesn't. A non-UUID string will produce an ActiveRecord::StatementInvalid from Postgres, which BaseController's rescue_from doesn't catch.
Nil safety in _balance.json.jbuilder (see inline) — Calling .format on money fields without &. will raise NoMethodError for balances where optional flow/start/end columns are NULL. Only cash_balance has the nil guard today.
Design
Pagination helpers duplicated (see inline) — safe_page_param, safe_per_page_param, and valid_uuid? are copied from AccountsController. These should be moved to BaseController.
Lambda in partial (see inline) — Style issue; the money_to_minor_units lambda should be a helper method.
Minor
- The
# Export materialized daily balances for backup and verification.comment indata_exporter.rbexplains what the code does, which the project conventions discourage. The surrounding blocks don't have equivalent comments — remove it for consistency. - No test for the
?currency=filter (onlyaccount_idand date range are tested). Worth adding given it's a distinct code path.
Generated by Claude Code
| private | ||
|
|
||
| def set_balance | ||
| @balance = balances_scope.find(params[:id]) |
There was a problem hiding this comment.
AccountsController#show explicitly validates the UUID before calling find to avoid a Postgres ActiveRecord::StatementInvalid when the primary key column is UUID type and the input is not a valid UUID string. That exception is not covered by the rescue_from ActiveRecord::RecordNotFound in BaseController, so a request like GET /api/v1/balances/not-a-uuid would return a 500 instead of a 404.
The same guard used in AccountsController should be applied here:
def set_balance
unless valid_uuid?(params[:id])
render json: { error: "not_found", message: "Balance not found" }, status: :not_found
return
end
@balance = balances_scope.find(params[:id])
endGenerated by Claude Code
JSONbored
force-pushed
the
codex/feat-api-balance-history
branch
from
5e9eda9 to
2f76a03
Compare
brin-security-scanner
Bot
added
the
contributor:flagged
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
spec/swagger_helper.rb (1)
245-272:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark the optional balance money fields nullable.
_balance.json.jbuildercan emitnullfor the start/end and flow breakdown fields when the underlying money is missing, so leaving them non-nullable makes the OpenAPI contract inaccurate. This is the same mismatch called out previously and it is still present here.Suggested fix
- start_cash_balance: { type: :string }, - start_cash_balance_cents: { type: :integer, description: 'Starting cash balance in currency minor units' }, + start_cash_balance: { type: :string, nullable: true }, + start_cash_balance_cents: { type: :integer, nullable: true, description: 'Starting cash balance in currency minor units' },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/swagger_helper.rb` around lines 245 - 272, The OpenAPI schema in swagger_helper.rb marks many balance and flow money fields as required but the jbuilder can emit null; update the property definitions for the optional money fields (e.g. start_cash_balance, start_cash_balance_cents, start_non_cash_balance, start_non_cash_balance_cents, start_balance, start_balance_cents, cash_inflows, cash_inflows_cents, cash_outflows, cash_outflows_cents, non_cash_inflows, non_cash_inflows_cents, non_cash_outflows, non_cash_outflows_cents, net_market_flows, net_market_flows_cents, cash_adjustments, cash_adjustments_cents, non_cash_adjustments, non_cash_adjustments_cents, end_cash_balance, end_cash_balance_cents, end_non_cash_balance, end_non_cash_balance_cents, end_balance, end_balance_cents) to include nullable: true so the OpenAPI contract matches the possible nulls emitted by _balance.json.jbuilder.
🧹 Nitpick comments (1)
spec/requests/api/v1/balances_spec.rb (1)
34-43: ⚡ Quick winKeep the API-key fixture creation consistent.
api_key_without_read_scopebypasses the standardApiKey.create!path used elsewhere in this rswag suite. That makes this auth example more brittle and can skip model callbacks/normalization that the real authentication flow depends on.Suggested fix
let(:api_key_without_read_scope) do key = ApiKey.generate_secure_key - ApiKey.new( + ApiKey.create!( user: user, name: 'No Read Docs Key', key: key, scopes: %w[write], source: 'web' - ).tap { |api_key| api_key.save!(validate: false) } + ) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/api/v1/balances_spec.rb` around lines 34 - 43, The fixture `api_key_without_read_scope` is creating the ApiKey via ApiKey.new(...).tap { ... save!(validate: false) } which bypasses the normal creation path and model callbacks; change the block to create the key through the standard creation path (call ApiKey.create! with the same attributes: user: user, name: 'No Read Docs Key', key: ApiKey.generate_secure_key, scopes: %w[write], source: 'web') so that callbacks/normalization run (update the `api_key_without_read_scope` let to use ApiKey.create! instead of new/save!).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/api/openapi.yaml`:
- Around line 2183-2188: The OpenAPI path parameter named "id" is declared only
as type: string but should be a UUID; update the parameter definition for id
(the balance ID path parameter) to declare it as a UUID by adding the UUID hint
(e.g., set schema: type: string and add format: uuid or an appropriate UUID
pattern) so generated clients and validation reflect the controller/tests that
expect UUIDs.
In `@spec/swagger_helper.rb`:
- Around line 273-275: The nested account property in the Balance schema
references the full Account schema (Account) which includes a required status
field that Balance endpoints do not emit; update the Balance schema so its
account field references a balance-specific schema (e.g., AccountSummary or
BalanceAccount) that only includes id, name, and account_type (and no required
status) or create such a new schema and point the account $ref to it, adjusting
the components/schemas accordingly so the documented response matches the actual
payload.
In `@test/controllers/api/v1/balances_controller_test.rb`:
- Around line 168-170: The api_headers helper currently sends the wrong
credential: update the api_headers method used in tests (def
api_headers(api_key)) to return the display key instead of the plain key by
replacing api_key.plain_key with api_key.display_key so X-Api-Key uses the same
credential value as production.
---
Duplicate comments:
In `@spec/swagger_helper.rb`:
- Around line 245-272: The OpenAPI schema in swagger_helper.rb marks many
balance and flow money fields as required but the jbuilder can emit null; update
the property definitions for the optional money fields (e.g. start_cash_balance,
start_cash_balance_cents, start_non_cash_balance, start_non_cash_balance_cents,
start_balance, start_balance_cents, cash_inflows, cash_inflows_cents,
cash_outflows, cash_outflows_cents, non_cash_inflows, non_cash_inflows_cents,
non_cash_outflows, non_cash_outflows_cents, net_market_flows,
net_market_flows_cents, cash_adjustments, cash_adjustments_cents,
non_cash_adjustments, non_cash_adjustments_cents, end_cash_balance,
end_cash_balance_cents, end_non_cash_balance, end_non_cash_balance_cents,
end_balance, end_balance_cents) to include nullable: true so the OpenAPI
contract matches the possible nulls emitted by _balance.json.jbuilder.
---
Nitpick comments:
In `@spec/requests/api/v1/balances_spec.rb`:
- Around line 34-43: The fixture `api_key_without_read_scope` is creating the
ApiKey via ApiKey.new(...).tap { ... save!(validate: false) } which bypasses the
normal creation path and model callbacks; change the block to create the key
through the standard creation path (call ApiKey.create! with the same
attributes: user: user, name: 'No Read Docs Key', key:
ApiKey.generate_secure_key, scopes: %w[write], source: 'web') so that
callbacks/normalization run (update the `api_key_without_read_scope` let to use
ApiKey.create! instead of new/save!).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d95f63bc-d6d4-453d-acd1-4941b0417814
📥 Commits
Reviewing files that changed from the base of the PR and between a8425a2 and 2f76a039af748dad332b2e0d482a76314ef133d2.
📒 Files selected for processing (12)
app/controllers/api/v1/balances_controller.rbapp/controllers/api/v1/base_controller.rbapp/models/family/data_exporter.rbapp/views/api/v1/balances/_balance.json.jbuilderapp/views/api/v1/balances/index.json.jbuilderapp/views/api/v1/balances/show.json.jbuilderconfig/routes.rbdocs/api/openapi.yamlspec/requests/api/v1/balances_spec.rbspec/swagger_helper.rbtest/controllers/api/v1/balances_controller_test.rbtest/models/family/data_exporter_test.rb
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/requests/api/v1/balances_spec.rb`:
- Around line 114-128: Consolidate the two duplicate response '422' blocks into
a single response '422' block that references the same schema ('$ref' =>
'#/components/schemas/ErrorResponse') and documents both failure cases—merge the
descriptions for "invalid account filter" and "invalid date filter" (or add
examples) so both are represented in the generated OpenAPI output; update the
specs around the existing response '422' block (keeping the let(:account_id) and
let(:start_date) test setup and run_test! examples) so you no longer have two
separate response '422' blocks (the ones labeled "invalid account filter" and
"invalid date filter").
In `@spec/swagger_helper.rb`:
- Around line 278-286: BalanceAccount schema in spec/swagger_helper.rb defines
account_type without nullable, making it stricter than the parent Account
schema; update the BalanceAccount properties to set account_type: { type:
:string, nullable: true } so it matches Account and prevents NoMethodError in
the jbuilder rendering (balance.account.accountable_type.underscore) and
downstream schema validation failures. Ensure the change is applied to the
BalanceAccount definition (the properties hash containing id, name,
account_type) so responses with nil accountable_type are rendered as null and
validate correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff6f4c18-78da-4ca7-b1cd-620e38e23a5e
📥 Commits
Reviewing files that changed from the base of the PR and between 2f76a039af748dad332b2e0d482a76314ef133d2 and d40bbd35c10f4db19080a1e2c22f7bf1efd10181.
📒 Files selected for processing (4)
docs/api/openapi.yamlspec/requests/api/v1/balances_spec.rbspec/swagger_helper.rbtest/controllers/api/v1/balances_controller_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- test/controllers/api/v1/balances_controller_test.rb
- docs/api/openapi.yaml
JSONbored
force-pushed
the
codex/feat-api-balance-history
branch
from
a7d2b87 to
3b29e63
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/family/data_exporter.rb`:
- Around line 157-160: The current use of
Balance.joins(:account).where(accounts: { family_id: `@family.id`
}).chronological.find_each loses the chronological ordering because find_each
batches by primary key; replace find_each with an explicit ordered iteration
that preserves .chronological — for example iterate with .each using a
limit/offset batching loop or implement a cursor-based loop using the
chronological key (e.g., date) to fetch the next batch, ensuring
Balance.joins(:account), .chronological and the `@family.id` scope remain and that
batches are fetched in chronological order rather than by primary key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e596d68-2719-4c3e-9a38-6dee93da93bd
📥 Commits
Reviewing files that changed from the base of the PR and between a7d2b87623e008f0c177a2955a4ae25f53cdf28e and 3b29e632f4621944550cd6aafe87264c4de3a0c5.
📒 Files selected for processing (12)
app/controllers/api/v1/balances_controller.rbapp/controllers/api/v1/base_controller.rbapp/models/family/data_exporter.rbapp/views/api/v1/balances/_balance.json.jbuilderapp/views/api/v1/balances/index.json.jbuilderapp/views/api/v1/balances/show.json.jbuilderconfig/routes.rbdocs/api/openapi.yamlspec/requests/api/v1/balances_spec.rbspec/swagger_helper.rbtest/controllers/api/v1/balances_controller_test.rbtest/models/family/data_exporter_test.rb
✅ Files skipped from review due to trivial changes (5)
- app/views/api/v1/balances/show.json.jbuilder
- spec/requests/api/v1/balances_spec.rb
- app/views/api/v1/balances/_balance.json.jbuilder
- docs/api/openapi.yaml
- spec/swagger_helper.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- test/models/family/data_exporter_test.rb
- app/views/api/v1/balances/index.json.jbuilder
- config/routes.rb
- test/controllers/api/v1/balances_controller_test.rb
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Follow-up review
The previous issues were addressed well overall:
- ✅ N+1 in the exporter fixed — now uses a single flat
Balance.joins(:account).where(...)query - ✅ Nil safety in the partial fixed —
format_moneyhelper with&.formatapplied consistently - ✅ UUID guard in
showfixed — raisesActiveRecord::RecordNotFoundwhichBaseControllerrescues cleanly - ✅ Pagination helpers,
InvalidFilterError,parse_date_param, andUUID_PATTERNextracted toBaseController— good improvement that benefits all subcontrollers - ✅ Lambda in partial replaced with proper
format_money/money_to_minor_unitshelper methods - ✅
BalanceAccountschema introduced to avoid misrepresenting the nested account payload - ✅ Currency filter test and malformed-ID test added
Two issues remain:
1. .chronological.find_each silently drops the ordering (see inline) — find_each ignores any scoped ORDER BY in Rails 7 and batches by primary key. The .chronological call is a no-op here. Since the full result set is buffered in lines anyway, switch to .each to make the ordering honest.
2. accountable_type.underscore can crash on nil (see inline) — The schema correctly marks account_type as nullable: true, but the template still calls .underscore without a nil guard, which would raise NoMethodError rather than rendering null.
Generated by Claude Code
| Balance.joins(:account) | ||
| .where(accounts: { family_id: @family.id }) | ||
| .chronological | ||
| .each do |balance| |
There was a problem hiding this comment.
.chronological applies ORDER BY date ASC to the relation, but find_each ignores any scoped order and batches by primary key instead. In Rails 7, without error_on_ignored_order = true (not set in this project), the chronological order is silently dropped. The records will be emitted in UUID primary key order, not date order.
Since the export accumulates everything into the lines array in memory anyway (same as all other sections in this file), the simplest fix that actually gives chronological output is to replace find_each with each:
Balance.joins(:account)
.where(accounts: { family_id: @family.id })
.chronological
.each do |balance|
lines << { ... }.to_json
endIf batching is needed for very large datasets, a cursor-based approach using in_batches with explicit PK ordering, then resorting each batch by date, would be required — but that's additional complexity. For an export that already buffers everything in lines, each is the honest choice here.
Generated by Claude Code
| json.end_balance_cents money_to_minor_units(balance.end_balance_money) | ||
|
|
||
| json.account do | ||
| json.id balance.account.id |
There was a problem hiding this comment.
accountable_type can be nil (the BalanceAccount schema correctly marks account_type as nullable: true), but calling .underscore on nil will raise NoMethodError at runtime rather than rendering null. Needs a safe-navigation guard:
json.account_type balance.account.accountable_type&.underscoreGenerated by Claude Code
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/models/family/data_exporter_test.rb (1)
342-351: Consider explicitly addingflows_factorto balance creations for clarity.While the
flows_factorcolumn has a database default of 1, explicitly setting it in the test data makes the intent clear and documents the expected behavior. This also protects against future schema changes.Optional improvement
balance = `@account.balances.create`!( date: Date.parse("2024-01-15"), balance: 1234.56, cash_balance: 1234.56, start_cash_balance: 1000, start_non_cash_balance: 0, cash_inflows: 234.56, cash_outflows: 0, + flows_factor: 1, currency: "USD" )- `@account.balances.create`!(date: Date.parse("2024-03-01"), balance: 300, currency: "USD") - `@account.balances.create`!(date: Date.parse("2024-01-01"), balance: 100, currency: "USD") + `@account.balances.create`!(date: Date.parse("2024-03-01"), balance: 300, currency: "USD", flows_factor: 1) + `@account.balances.create`!(date: Date.parse("2024-01-01"), balance: 100, currency: "USD", flows_factor: 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/family/data_exporter_test.rb` around lines 342 - 351, Add an explicit flows_factor when creating test balances to document intent and guard against schema changes: update the `@account.balances.create`! call (the balance creation block that assigns to local variable balance) to include flows_factor: 1 alongside the other attributes (date, balance, cash_balance, etc.) so the test data explicitly reflects the expected default behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/models/family/data_exporter_test.rb`:
- Around line 342-351: Add an explicit flows_factor when creating test balances
to document intent and guard against schema changes: update the
`@account.balances.create`! call (the balance creation block that assigns to local
variable balance) to include flows_factor: 1 alongside the other attributes
(date, balance, cash_balance, etc.) so the test data explicitly reflects the
expected default behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 617eecf7-1891-41a4-b746-9affe9e6bd2c
📥 Commits
Reviewing files that changed from the base of the PR and between 3b29e632f4621944550cd6aafe87264c4de3a0c5 and 0fdbdfba94705ef0e7800cec8ded695fd535103b.
📒 Files selected for processing (2)
app/models/family/data_exporter.rbtest/models/family/data_exporter_test.rb
jjmata
removed
the
contributor:flagged
JSONbored
force-pushed
the
codex/feat-api-balance-history
branch
from
a51c300 to
d732bf5
Compare
brin-security-scanner
Bot
added
the
contributor:flagged
Thanks for the follow-up. The two remaining items are addressed now:
I also addressed CodeRabbit’s test nit by making the exporter balance test data set |
jjmata
removed
the
contributor:flagged
|
Can you resolve the merge conflict @JSONbored? 🙏 |
brin-security-scanner
Bot
added
the
contributor:flagged
jjmata
removed
the
contributor:flagged
2 participants
Summary
What changed
GET /api/v1/balancesandGET /api/v1/balances/:idwith account, currency, and date filters.Balancerecords inall.ndjsonwith account linkage and computed flow fields.docs/api/openapi.yaml.Why
Validation
docker exec -w /workspace devcontainer-app-1 bin/rails test test/controllers/api/v1/balances_controller_test.rb test/models/family/data_exporter_test.rbdocker exec -w /workspace devcontainer-app-1 env RAILS_ENV=test bundle exec rake rswag:specs:swaggerizedocker exec -w /workspace devcontainer-app-1 bundle exec rubocop app/controllers/api/v1/balances_controller.rb app/models/family/data_exporter.rb test/controllers/api/v1/balances_controller_test.rb test/models/family/data_exporter_test.rb spec/requests/api/v1/balances_spec.rb spec/swagger_helper.rbgit diff --checkNotes
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Documentation
Tests