feat(api): expose complete account export state#1597
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 an accounts Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Api::V1::AccountsController
participant DB as ActiveRecord::Account
participant View as Jbuilder/_account
Client->>API: GET /api/v1/accounts/:id?include_disabled=true (X-Api-Key)
API->>API: valid_uuid?(params[:id])
alt uuid invalid
API-->>Client: 404 JSON (not_found)
else uuid valid
API->>DB: accounts_scope.find(params[:id])
alt found
DB-->>API: account record
API->>View: render :show with `@account`
View-->>Client: 200 JSON (AccountDetail)
else not found
DB-->>API: ActiveRecord::RecordNotFound
API-->>Client: 404 JSON (not_found)
end
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
the
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
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: debcacb8ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/controllers/api/v1/accounts_controller.rb`:
- Line 10: The index/export query currently sets accounts_query =
accounts_scope.alphabetically but does not eager-load associations used by
_account.json.jbuilder (account.subtype, institution_name, institution_domain)
which traverse Account#accountable and its provider fallback; change the query
to eager-load those associations before pagination (e.g. use
accounts_scope.includes(accountable: :provider).alphabetically or the equivalent
includes for your polymorphic types) so that Account#accountable and the
provider fallback are loaded in a single query rather than N+1 per account.
In `@spec/swagger_helper.rb`:
- Around line 172-180: The shared Account schema now requires status but nested
account objects in Transaction/Valuation/Holding/Trade/Transfer endpoints don’t
emit it; fix by either (A) removing status from the required array in the
Account schema (symbol: Account) so the lightweight representation omits status,
or (B) update the endpoint schemas/partials that render full account data to
reference the detailed schema (symbol: AccountDetail / the dedicated accounts
partial) instead of Account so they include status; pick one approach and update
swagger_helper.rb and the endpoint schema references to keep contract
consistent.
In `@test/controllers/api/v1/accounts_controller_test.rb`:
- Around line 126-160: Add two missing show-action tests: one that requests a
non-existent account ID and asserts a 404 response, and one that calls GET
"/api/v1/accounts/:id" without passing api_headers (or with missing/invalid
`@api_key`) and asserts a 401 response. Place them alongside the existing tests
(e.g. create tests named like "should return 404 for unknown account" and
"should return 401 when auth missing on show"), use the same request pattern as
other tests (GET "/api/v1/accounts/#{id}" with/without params and headers) and
parse/assert response status consistently with the existing tests.
- Around line 17-23: The test suite still uses Doorkeeper bearer tokens; update
all API v1 account controller tests to use ApiKey.create! and
api_headers(api_key) instead: replace any OAuth/Bearer auth setup with creating
ApiKey records via ApiKey.create!(user: `@user`, name: "...", scopes: ["read"] or
["read","read_write"] as needed, display_key: SecureRandom-generated value) and
pass headers using api_headers(api_key) (which returns { "X-Api-Key" =>
api_key.display_key }) when making requests; update the blocks around the
existing ApiKey creation (e.g., the `@user.api_keys.active.destroy_all` /
ApiKey.create! lines and the similar cases at 276-280) to follow this pattern
for all tests in the file.
🪄 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: 96aaadd1-e759-4521-bfac-b1dcad5be379
📥 Commits
Reviewing files that changed from the base of the PR and between c91b730 and debcacb8ae70edfa4d259b060d794dfea0dc80b8.
📒 Files selected for processing (8)
app/controllers/api/v1/accounts_controller.rbapp/views/api/v1/accounts/_account.json.jbuilderapp/views/api/v1/accounts/index.json.jbuilderapp/views/api/v1/accounts/show.json.jbuilderdocs/api/openapi.yamlspec/requests/api/v1/accounts_spec.rbspec/swagger_helper.rbtest/controllers/api/v1/accounts_controller_test.rb
JSONbored
force-pushed
the
feat/api-account-export-parity
branch
from
debcacb to
8a03891
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a038913e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/api/openapi.yaml (1)
997-999: ⚡ Quick winDocument
idas UUID format for the new show endpoint.Since this path is UUID-based, adding
format: uuidhere improves generated client validation and keeps the contract explicit.Proposed diff
- name: id in: path required: true description: Account ID schema: type: string + format: uuid🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api/openapi.yaml` around lines 997 - 999, The OpenAPI schema for the show endpoint's path parameter "id" currently only declares type: string; update the parameter schema for the GET show endpoint to include format: uuid so generated clients validate UUIDs; locate the GET operation (the "get" for the show endpoint) and the parameter named "id" in docs/api/openapi.yaml and add format: uuid alongside type: string in that parameter's schema.test/controllers/api/v1/accounts_controller_test.rb (1)
108-119: ⚡ Quick winStrengthen show assertions for the full export payload.
Given this PR’s “complete account export state” goal, consider asserting the remaining newly exposed fields (
institution_name,institution_domain,created_at,updated_at) in the show success case so regressions are caught early.Proposed diff
test "should show active account" do account = accounts(:depository) get "/api/v1/accounts/#{account.id}", headers: api_headers(`@api_key`) assert_response :success response_body = JSON.parse(response.body) assert_equal account.id, response_body["id"] assert_equal account.status, response_body["status"] assert response_body.key?("cash_balance") assert response_body.key?("subtype") + assert response_body.key?("institution_name") + assert response_body.key?("institution_domain") + assert response_body.key?("created_at") + assert response_body.key?("updated_at") endAs per coding guidelines: "All new or changed API v1 endpoint actions must have Minitest behavioral coverage in
test/controllers/api/v1/{resource}_controller_test.rb."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/api/v1/accounts_controller_test.rb` around lines 108 - 119, Update the "should show active account" test to assert the newly exported fields: after parsing response_body (response_body variable) add assertions that response_body["institution_name"] and response_body["institution_domain"] match account.institution_name and account.institution_domain, and assert that response_body includes created_at and updated_at (either by checking keys or comparing to account.created_at.iso8601 / account.updated_at.iso8601) so the test for accounts(:depository) called via get "/api/v1/accounts/#{account.id}" with api_headers(`@api_key`) verifies these new fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/api/openapi.yaml`:
- Around line 997-999: The OpenAPI schema for the show endpoint's path parameter
"id" currently only declares type: string; update the parameter schema for the
GET show endpoint to include format: uuid so generated clients validate UUIDs;
locate the GET operation (the "get" for the show endpoint) and the parameter
named "id" in docs/api/openapi.yaml and add format: uuid alongside type: string
in that parameter's schema.
In `@test/controllers/api/v1/accounts_controller_test.rb`:
- Around line 108-119: Update the "should show active account" test to assert
the newly exported fields: after parsing response_body (response_body variable)
add assertions that response_body["institution_name"] and
response_body["institution_domain"] match account.institution_name and
account.institution_domain, and assert that response_body includes created_at
and updated_at (either by checking keys or comparing to
account.created_at.iso8601 / account.updated_at.iso8601) so the test for
accounts(:depository) called via get "/api/v1/accounts/#{account.id}" with
api_headers(`@api_key`) verifies these new fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d1854f3-7328-4645-a09d-af8e08ec04b3
📥 Commits
Reviewing files that changed from the base of the PR and between debcacb8ae70edfa4d259b060d794dfea0dc80b8 and 3257264b519e975fd5e924d8662ff03078b97fee.
📒 Files selected for processing (8)
app/controllers/api/v1/accounts_controller.rbapp/views/api/v1/accounts/_account.json.jbuilderapp/views/api/v1/accounts/index.json.jbuilderapp/views/api/v1/accounts/show.json.jbuilderdocs/api/openapi.yamlspec/requests/api/v1/accounts_spec.rbspec/swagger_helper.rbtest/controllers/api/v1/accounts_controller_test.rb
✅ Files skipped from review due to trivial changes (2)
- app/views/api/v1/accounts/_account.json.jbuilder
- app/views/api/v1/accounts/show.json.jbuilder
🚧 Files skipped from review as they are similar to previous changes (2)
- app/controllers/api/v1/accounts_controller.rb
- spec/requests/api/v1/accounts_spec.rb
jjmata
removed
the
contributor:flagged
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Claude Code reivew that is wrong in the first assesment, so keep aware:
PR #1597 Review — feat(api): expose complete account export state
Overview
This PR adds a GET /api/v1/accounts/{id} show endpoint, expands the account JSON response with status, cash_balance, subtype, institution_name/domain, and timestamps, adds an include_disabled query param to both index and show, and migrates the test suite from Doorkeeper OAuth tokens to API key auth. The goals are valid; the implementation has several correctness issues that need fixing before merge.
Bugs / Correctness Issues
1. ApiKey.create! will fail validation in setup — tests are currently broken
The test setup creates ApiKey records without a source field:
@api_key = ApiKey.create!(
user: @user,
name: "Test Read Key",
scopes: [ "read" ],
display_key: "test_read_#{SecureRandom.hex(8)}"
)The model has validates :source, presence: true, inclusion: { in: SOURCES } where SOURCES = ["web", "mobile", "monitoring"]. This create! will raise ActiveRecord::RecordInvalid and fail every test in the file. Add source: "web" (or appropriate value) to both create! calls.
2. save!(validate: false) to test scope enforcement
The "should require read_accounts scope" test uses save!(validate: false) to bypass the scopes_not_empty validation. Besides being a code smell, it also bypasses the source validation, leaving a record with source: nil. Create the key with valid attributes and explicitly set scopes: ["read_write"] (or another valid non-read scope) to test rejection — no need to bypass validations.
3. Eager loading is missing in show, present in index
The index action adds .includes(:accountable, account_providers: :provider), but show uses bare accounts_scope.find(...). The shared _account partial renders institution_name and institution_domain, which likely traverse the account_providers -> provider association. This means show will fire additional queries per record that index avoids. Move the includes into accounts_scope, or add a separate show-specific scope.
Code Quality Issues
4. Controller indentation is inconsistent
The private block is indented 4 spaces while the public action methods (def index, def show) are at 2 spaces. In Ruby/Rails convention, private should be at the same indentation level as the public defs. This is visible in the diff:
end # end of show — 2-space indent
private # ← 4-space indent (wrong)
def ensure_read_scope # ← 6-space indent5. balance and cash_balance are formatted strings, not numeric values
account.balance_money.format returns a locale-formatted string like "$1,234.56". The stated purpose of this endpoint is export, migration, and reconciliation tooling. Integrations need a raw numeric value, not a display string. Expose amount (numeric) and optionally formatted_balance separately, consistent with how other API responses in the codebase handle money.
6. Missing 401/403 responses in OpenAPI spec for show
The show endpoint requires auth (before_action :ensure_read_scope), but the OpenAPI spec only documents 200 and 404. The 401 (unauthenticated) and 403 (insufficient scope) cases are missing from both the YAML and the rswag spec. Other endpoints in openapi.yaml document these — consistency matters for generated clients.
Minor / Style
7. valid_uuid? guard is reasonable but could be simplified
The UUID pre-check before find is a valid approach to prevent ActiveRecord::StatementInvalid from PostgreSQL when a non-UUID string is passed to a UUID primary key column. However, rescuing ActiveRecord::StatementInvalid alongside RecordNotFound is the more idiomatic Rails pattern and removes the need for a custom regex helper.
8. api_headers helper should use plain_key per project convention
CLAUDE.md specifies api_key.plain_key for the X-Api-Key header value. In this model plain_key is just an alias for display_key, so it's functionally identical — but using plain_key matches the documented convention and makes intent clearer.
9. subtype field is not verified to exist on Account
The partial calls account.subtype. If this is delegated to the accountable (e.g., Depository#subtype), confirm it exists on all accountable types. A NoMethodError here would fall through to the 500 rescue path silently in production.
Test Coverage — What's Good
The new Minitest tests cover the important cases well: active show, 404 for unknown UUID, 404 for malformed ID, auth required, disabled-hidden-by-default, disabled-visible-with-param, and cross-family isolation. That coverage matches project expectations.
Summary
The two blocking issues are the missing source: in ApiKey.create! (tests can't pass as written) and the eager loading gap between index and show. The formatted money strings are a design concern worth discussing with maintainers given the export intent. Everything else is style or documentation cleanup.
JSONbored
force-pushed
the
feat/api-account-export-parity
branch
from
3257264 to
94617dd
Compare
brin-security-scanner
Bot
added
the
contributor:flagged
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/api/openapi.yaml (1)
1565-1582: 💤 Low valueMissing 401 response body schema for balance_sheet endpoint.
The 401 response is documented but lacks a content schema, unlike other endpoints that reference
ErrorResponse. This inconsistency could confuse API consumers.📝 Suggested fix for consistency
'401': description: unauthorized + content: + application/json: + schema: + "$ref": "#/components/schemas/ErrorResponse"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api/openapi.yaml` around lines 1565 - 1582, The 401 response for the GET /api/v1/balance_sheet operation is missing a response body schema; update the GET operation under "/api/v1/balance_sheet" so the '401' response includes a content/application/json block that references the existing ErrorResponse schema (e.g., "$ref": "#/components/schemas/ErrorResponse") to match other endpoints' documentation.
🤖 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 980-988: For the path "/api/v1/accounts/{id}" update the parameter
definition for name: id so the format: uuid is nested inside its schema;
specifically, move the existing top-level "format: uuid" into the "schema"
object (which already has type: string) so the schema reads type: string and
format: uuid; ensure required, in, description and name remain unchanged;
validate the OpenAPI after change.
---
Nitpick comments:
In `@docs/api/openapi.yaml`:
- Around line 1565-1582: The 401 response for the GET /api/v1/balance_sheet
operation is missing a response body schema; update the GET operation under
"/api/v1/balance_sheet" so the '401' response includes a
content/application/json block that references the existing ErrorResponse schema
(e.g., "$ref": "#/components/schemas/ErrorResponse") to match other endpoints'
documentation.
🪄 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: d038183a-e8af-4a30-9863-b7cd7ba8b4e5
📥 Commits
Reviewing files that changed from the base of the PR and between 3257264b519e975fd5e924d8662ff03078b97fee and 94617dd.
📒 Files selected for processing (8)
app/controllers/api/v1/accounts_controller.rbapp/views/api/v1/accounts/_account.json.jbuilderapp/views/api/v1/accounts/index.json.jbuilderapp/views/api/v1/accounts/show.json.jbuilderdocs/api/openapi.yamlspec/requests/api/v1/accounts_spec.rbspec/swagger_helper.rbtest/controllers/api/v1/accounts_controller_test.rb
✅ Files skipped from review due to trivial changes (3)
- app/views/api/v1/accounts/show.json.jbuilder
- app/views/api/v1/accounts/_account.json.jbuilder
- app/controllers/api/v1/accounts_controller.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/views/api/v1/accounts/index.json.jbuilder
- spec/swagger_helper.rb
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 1396-1401: The 401 response for the sso_link endpoint is
documented as the generic ErrorResponse but Api::V1::AuthController#sso_link can
return an MFA-specific payload (mfa_required: true); update the OpenAPI contract
so the 401 response for sso_link references a new schema (e.g.,
MfaRequiredResponse) that includes the mfa_required boolean and any additional
fields, and add that new schema to components.schemas instead of using
ErrorResponse so clients can detect the MFA branch; locate the sso_link
operation's 401 entry and replace the "$ref":
"#/components/schemas/ErrorResponse" with a reference to the new
MfaRequiredResponse schema (and add examples if available).
- Around line 922-938: The OpenAPI BalanceSheet schema marks "currency" as
required but the controller Api::V1::BalanceSheetController#show does not
include currency in its response; update either the implementation or the
specification to match: either add currency to the controller response payload
returned by Api::V1::BalanceSheetController#show (populate it with the family's
primary currency) or remove "currency" from the BalanceSheet schema's required
list and properties so generated clients reflect the actual net_worth, assets,
and liabilities fields. Ensure the chosen change keeps the "$ref" references to
Money for net_worth, assets, and liabilities intact.
In `@spec/requests/api/v1/accounts_spec.rb`:
- Around line 34-43: The fixture api_key_without_read_scope is using scopes: []
which authorize_scope! treats as blank/unrestricted; change the scopes to a
nonblank scope that does not include read (for example scopes: ['write'] or
another app-appropriate non-read scope) so authorize_scope! will deny read
access and the 403 example exercises the failure path; update the ApiKey
creation in the api_key_without_read_scope let block accordingly.
🪄 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: da69d20f-2155-4c67-a99a-a85dfd96b9b8
📒 Files selected for processing (2)
docs/api/openapi.yamlspec/requests/api/v1/accounts_spec.rb
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/api/openapi.yaml (1)
1586-1594: 💤 Low valueMissing response schema for 401 on balance_sheet endpoint.
The 401 response at line 1593-1594 only has a description without a content/schema definition, unlike other authenticated endpoints (e.g., accounts at lines 1019-1024). For consistency and accurate client code generation, consider adding the ErrorResponse schema.
🔧 Suggested fix
'401': description: unauthorized + content: + application/json: + schema: + "$ref": "#/components/schemas/ErrorResponse"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api/openapi.yaml` around lines 1586 - 1594, The 401 response for the balance_sheet endpoint is missing a response schema; update the balance_sheet endpoint's '401' response to include a content/application/json schema referencing the existing ErrorResponse component (e.g., add content: application/json: schema: "$ref": "#/components/schemas/ErrorResponse") so it matches other authenticated endpoints like accounts and supports accurate client generation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/api/openapi.yaml`:
- Around line 1586-1594: The 401 response for the balance_sheet endpoint is
missing a response schema; update the balance_sheet endpoint's '401' response to
include a content/application/json schema referencing the existing ErrorResponse
component (e.g., add content: application/json: schema: "$ref":
"#/components/schemas/ErrorResponse") so it matches other authenticated
endpoints like accounts and supports accurate client generation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfe26ed9-d34a-4094-950e-75533048d08e
📒 Files selected for processing (4)
docs/api/openapi.yamlspec/requests/api/v1/accounts_spec.rbspec/requests/api/v1/auth_spec.rbspec/swagger_helper.rb
✅ Files skipped from review due to trivial changes (1)
- spec/requests/api/v1/accounts_spec.rb
jjmata
removed
the
contributor:flagged
github-project-automation
Bot
moved this from In Progress
to Done
in Latest release train (v0.7.1) - May 2026
2 participants
Summary
What changed
showaction forGET /api/v1/accounts/{id}.include_disabled=trueto account index/show queries.Why
Current account API responses are too thin for reliable export, verification, and migration tooling. Integrations need account state beyond the basic row fields, especially when reconciling disabled/manual accounts or preserving account subtypes.
Validation
/usr/bin/ruby -c app/controllers/api/v1/accounts_controller.rb/usr/bin/ruby -c test/controllers/api/v1/accounts_controller_test.rb/usr/bin/ruby -c spec/requests/api/v1/accounts_spec.rbruby -e 'require "yaml"; YAML.load_file("docs/api/openapi.yaml")'git diff --checkBlocked locally:
bin/rails test ...andbin/rubocoprequire the repository Ruby/Bundler toolchain. This workstation currently exposes Ruby 2.6.10, while Sure requires Ruby 3.4.7 and Bundler 2.6.7.Notes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests