fix(accounts): preserve closed-account financial history#1730
Open
JSONbored wants to merge 8 commits into
Open
fix(accounts): preserve closed-account financial history#1730JSONbored wants to merge 8 commits into
JSONbored wants to merge 8 commits into
Conversation
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdd historical account scoping and UUID-filter validation: controllers use ChangesDisabled Account History Support
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
Collaborator
|
Is this really still a draft, @JSONbored? |
Keep disabled accounts hidden from default account navigation while preserving their historical ledger data for API portability and reporting. Update balance-sheet history, API read scopes, docs, OpenAPI specs, and tests so pending-deletion accounts remain excluded.
JSONbored
force-pushed
the
codex/docs-closed-account-portability
branch
from
a6a9025 to
44e814b
Compare
JSONbored
changed the title
superagent-security
Bot
added
contributor:verified
Contributor
Author
Haha my bad, the plan for this one changed slightly, however working on final pass improvements now. Closes out an existing old issue as well. |
![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: 44e814b9b7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
app/controllers/api/v1/transactions_controller.rb (5)
195-198:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message pattern inconsistency with project standard.
The
internal_server_errorresponse uses"An unexpected error occurred"instead of the established pattern"Error: #{e.message}". Based on learnings, API v1 controllers should consistently expose error messages in 500 responses usingmessage: "Error: #{e.message}".🔧 Proposed fix
render json: { error: "internal_server_error", - message: "An unexpected error occurred" + message: "Error: #{e.message}" }, status: :internal_server_error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/transactions_controller.rb` around lines 195 - 198, The 500-response render in TransactionsController uses a generic message; change the render json block (the hash passed to render json: with keys error and message) to use the exception message pattern used across API v1 by setting message to "Error: #{e.message}" (ensure this is inside the rescue block where `e` is the caught exception, e.g., in the rescue in TransactionsController action or rescue_from handler so `e` is in scope).
172-175:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message pattern inconsistency with project standard.
The
internal_server_errorresponse uses"An unexpected error occurred"instead of the established pattern"Error: #{e.message}". Based on learnings, API v1 controllers should consistently expose error messages in 500 responses usingmessage: "Error: #{e.message}".🔧 Proposed fix
render json: { error: "internal_server_error", - message: "An unexpected error occurred" + message: "Error: #{e.message}" }, status: :internal_server_error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/transactions_controller.rb` around lines 172 - 175, The 500 response in the render block inside TransactionsController currently returns a generic message; update the error response to follow the project pattern by using the exception's message (i.e., set message to "Error: #{e.message}") in the render json call that returns status: :internal_server_error so the controller (transactions_controller.rb) exposes the actual error via the rescue e variable.
51-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message pattern inconsistency with project standard.
The
internal_server_errorresponse uses"An unexpected error occurred"instead of the established pattern"Error: #{e.message}". Based on learnings, API v1 controllers should consistently expose error messages in 500 responses usingmessage: "Error: #{e.message}".🔧 Proposed fix
render json: { error: "internal_server_error", - message: "An unexpected error occurred" + message: "Error: #{e.message}" }, status: :internal_server_error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/transactions_controller.rb` around lines 51 - 54, Update the 500 error response in API v1 transactions controller to follow the project pattern by returning the exception message; locate the rescue block in app/controllers/api/v1/transactions_controller.rb (the block that calls render json: { error: "internal_server_error", ... }, status: :internal_server_error) and change the message field to use the rescued exception (message: "Error: #{e.message}") ensuring the rescue uses `rescue => e` so `e` is available.
126-129:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message pattern inconsistency with project standard.
The
internal_server_errorresponse uses"An unexpected error occurred"instead of the established pattern"Error: #{e.message}". Based on learnings, API v1 controllers should consistently expose error messages in 500 responses usingmessage: "Error: #{e.message}".🔧 Proposed fix
render json: { error: "internal_server_error", - message: "An unexpected error occurred" + message: "Error: #{e.message}" }, status: :internal_server_error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/transactions_controller.rb` around lines 126 - 129, Replace the static 500 response message with the project's standard error pattern by using the exception's message: in the TransactionsController rescue block where you call render json: { error: "internal_server_error", message: ... }, change the message to "Error: #{e.message}" (keep the same status: :internal_server_error and the error key), ensuring you reference the exception variable `e` used in that rescue so the rendered JSON follows the API v1 standard.
65-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message pattern inconsistency with project standard.
The
internal_server_errorresponse uses"An unexpected error occurred"instead of the established pattern"Error: #{e.message}". Based on learnings, API v1 controllers should consistently expose error messages in 500 responses usingmessage: "Error: #{e.message}".🔧 Proposed fix
render json: { error: "internal_server_error", - message: "An unexpected error occurred" + message: "Error: #{e.message}" }, status: :internal_server_error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/transactions_controller.rb` around lines 65 - 68, The 500 response in transactions_controller's error handler should follow the project pattern by exposing the exception message; update the render json block that currently returns message: "An unexpected error occurred" to use message: "Error: #{e.message}" (ensure this is inside the rescue that captures the exception as e) while keeping status: :internal_server_error and the error key "internal_server_error".app/controllers/api/v1/accounts_controller.rb (2)
50-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message pattern inconsistency with project standard.
The
internal_server_errorresponse uses"An unexpected error occurred"instead of the established pattern"Error: #{e.message}". Based on learnings, API v1 controllers should consistently expose error messages in 500 responses usingmessage: "Error: #{e.message}".🔧 Proposed fix
render json: { error: "internal_server_error", - message: "An unexpected error occurred" + message: "Error: #{e.message}" }, status: :internal_server_error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/accounts_controller.rb` around lines 50 - 53, Change the 500 response in AccountsController so it follows the project standard by returning the exception message instead of the static string: replace the current render json block that sets message: "An unexpected error occurred" with one that sets message: "Error: #{e.message}" (ensure this is inside the rescue that has access to the exception variable `e` and update the render call in AccountsController where the internal_server_error JSON is returned).
23-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message pattern inconsistency with project standard.
The
internal_server_errorresponse uses"An unexpected error occurred"instead of the established pattern"Error: #{e.message}". Based on learnings, API v1 controllers should consistently expose error messages in 500 responses usingmessage: "Error: #{e.message}".🔧 Proposed fix
render json: { error: "internal_server_error", - message: "An unexpected error occurred" + message: "Error: #{e.message}" }, status: :internal_server_error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/accounts_controller.rb` around lines 23 - 26, The 500 response in Api::V1::AccountsController currently returns a generic message; update the render json call in the controller's rescue/exception handling to use the project standard by setting message: "Error: #{e.message}" (keeping error: "internal_server_error" and status: :internal_server_error) and ensure the exception variable e is the one caught in that rescue block.app/controllers/api/v1/valuations_controller.rb (3)
43-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message pattern inconsistency with project standard.
The
internal_server_errorresponse uses"An unexpected error occurred"instead of the established pattern"Error: #{e.message}". Based on learnings, API v1 controllers should consistently expose error messages in 500 responses usingmessage: "Error: #{e.message}".🔧 Proposed fix
render json: { error: "internal_server_error", - message: "An unexpected error occurred" + message: "Error: #{e.message}" }, status: :internal_server_error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/valuations_controller.rb` around lines 43 - 46, The 500 response in ValuationsController currently renders a generic message; update the rescue block to render the project-standard pattern by returning message: "Error: #{e.message}" (ensure the rescue uses rescue => e so the exception variable e is available) while keeping error: "internal_server_error" and status: :internal_server_error; locate the render call in the controller (e.g., inside the rescue in the action or around method handle) and replace the static message with the interpolated one.
148-151:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message pattern inconsistency with project standard.
The
internal_server_errorresponse uses"An unexpected error occurred"instead of the established pattern"Error: #{e.message}". Based on learnings, API v1 controllers should consistently expose error messages in 500 responses usingmessage: "Error: #{e.message}".🔧 Proposed fix
render json: { error: "internal_server_error", - message: "An unexpected error occurred" + message: "Error: #{e.message}" }, status: :internal_server_error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/valuations_controller.rb` around lines 148 - 151, The 500 response in ValuationsController is returning a hardcoded message instead of the project-standard error pattern; update the rescue/render block (the code that currently does render json: { error: "internal_server_error", message: "An unexpected error occurred" }, status: :internal_server_error) to interpolate the exception message as message: "Error: #{e.message}" so the response becomes { error: "internal_server_error", message: "Error: #{e.message}" } while keeping status: :internal_server_error and using the same rescue variable (e) from the surrounding rescue handler.
55-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message pattern inconsistency with project standard.
The
internal_server_errorresponse uses"An unexpected error occurred"instead of the established pattern"Error: #{e.message}". Based on learnings, API v1 controllers should consistently expose error messages in 500 responses usingmessage: "Error: #{e.message}".🔧 Proposed fix
render json: { error: "internal_server_error", - message: "An unexpected error occurred" + message: "Error: #{e.message}" }, status: :internal_server_error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/valuations_controller.rb` around lines 55 - 58, The 500 response in the ValuationsController is using a generic message; update the render call in the rescue/exception handling block (where render json: { error: "internal_server_error", message: ... }, status: :internal_server_error) to follow project convention by interpolating the exception message (message: "Error: #{e.message}") so the JSON response exposes the actual error string; locate the rescue block in ValuationsController (the method handling the request or the around/rescue_from handler) and replace the static "An unexpected error occurred" with the interpolated error message.app/controllers/api/v1/trades_controller.rb (1)
322-326:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message pattern inconsistency with project standard.
The
internal_server_errorresponse uses"An unexpected error occurred"instead of the established pattern"Error: #{e.message}". Based on learnings, API v1 controllers should consistently expose error messages in 500 responses usingmessage: "Error: #{e.message}".🔧 Proposed fix
render json: { error: "internal_server_error", - message: "An unexpected error occurred" + message: "Error: #{e.message}" }, status: :internal_server_error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/trades_controller.rb` around lines 322 - 326, Replace the hardcoded 500 response message with the project-standard error pattern so the rescue block in TradesController returns message: "Error: #{e.message}"; locate the rescue/ensure handling around the action methods in app/controllers/api/v1/trades_controller.rb (the block that currently renders error: "internal_server_error") and update the JSON response to use the exception object e to build the message string while keeping status: :internal_server_error.app/controllers/api/v1/holdings_controller.rb (1)
123-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message pattern inconsistency with project standard.
The
internal_server_errorresponse uses"An unexpected error occurred"instead of the established pattern"Error: #{e.message}". Based on learnings, API v1 controllers should consistently expose error messages in 500 responses usingmessage: "Error: #{e.message}".🔧 Proposed fix
render json: { error: "internal_server_error", - message: "An unexpected error occurred" + message: "Error: #{e.message}" }, status: :internal_server_error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/holdings_controller.rb` around lines 123 - 126, The 500 response in HoldingsController currently returns a generic message; update the render in the rescue/exception handling block so it follows the project pattern by using the exception message (e.g., render json: { error: "internal_server_error", message: "Error: #{e.message}" }, status: :internal_server_error), ensuring you reference the caught exception variable (e) in the rescue block and keep the same error key and status.
🧹 Nitpick comments (1)
db/migrate/20260514090000_restore_trade_category_reference.rb (1)
13-13: ⚡ Quick winUpdate
downto match theon_deletespecification.If the
upmethod is updated to includeon_delete: :nullifyin the foreign key options, thedownmethod should mirror that specification for consistency, even thoughremove_referencewill drop the constraint regardless.🔧 Proposed fix
- remove_reference :trades, :category, foreign_key: true, type: :uuid + remove_reference :trades, :category, foreign_key: { on_delete: :nullify }, type: :uuid🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/migrate/20260514090000_restore_trade_category_reference.rb` at line 13, The migration's down method uses remove_reference :trades, :category, foreign_key: true, type: :uuid but should mirror the up method's foreign key options; update the remove_reference call to include the same on_delete option so the foreign key spec matches (e.g., change foreign_key: true to foreign_key: { on_delete: :nullify }) for the remove_reference :trades, :category invocation in the migration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/api/v1/balance_sheet_controller.rb`:
- Around line 32-34: The include_disabled_accounts? helper uses
ActiveModel::Type::Boolean#cast which can return nil for missing/blank params;
change it to return a strict boolean (false by default) by coercing the cast
result to a boolean (e.g., using !! or fallback to false) so the API returns
include_disabled: false when the param is absent; update the
include_disabled_accounts? method to use this coercion and ensure
Family#balance_sheet sees a true/false value rather than nil.
In `@db/migrate/20260514090000_restore_trade_category_reference.rb`:
- Line 7: Update the migration line that adds the category reference on trades
so the foreign key uses on_delete: :nullify instead of the default RESTRICT;
specifically change the add_reference call that currently reads add_reference
:trades, :category, null: true, foreign_key: true, type: :uuid to specify the
foreign_key option as a hash with on_delete: :nullify (e.g. foreign_key: {
on_delete: :nullify }) so deleting a Category nullifies trades.category_id to
match transactions.category_id behavior.
In `@docs/api/openapi.yaml`:
- Around line 4413-4414: The 422 response description "invalid date filter" is
too narrow; update the OpenAPI '422' response description entries (the response
object keyed by '422' in the YAML) to a broader phrasing such as "invalid or
malformed account filter" or "invalid filter" so it covers malformed account
filters as well; make the same change for the other occurrence referenced around
lines 6318-6319 by locating the other '422' response objects and replacing their
descriptions with the broader wording.
In `@spec/requests/api/v1/holdings_spec.rb`:
- Around line 107-113: Update the request spec so the account filter parameter
advertises a UUID rather than a generic string: in the spec around the response
'422', 'invalid account filter' (and the parameter definition used by the
holdings spec) change the parameter schema for account_id to require a UUID
(e.g., schema type:string with format:uuid or an appropriate UUID pattern) so
generated OpenAPI contract reflects the new 422 malformed-account-id behavior.
In `@test/controllers/api/v1/balance_sheet_controller_test.rb`:
- Line 35: The test currently only asserts presence of the "include_disabled"
key; update the assertion to also enforce the default value by adding an
equality assertion such as assert_equal false, response_body["include_disabled"]
(or assert_not response_body["include_disabled"]) immediately after the existing
assert response_body.key?("include_disabled") in
test/controllers/api/v1/balance_sheet_controller_test.rb so the default contract
for include_disabled is explicitly verified.
In `@test/controllers/api/v1/holdings_controller_test.rb`:
- Around line 32-85: Add tests covering unauthenticated (401) and
insufficient-scope (403) cases for both index and show: create two tests that
call get api_v1_holdings_url and get api_v1_holding_url(`@holding`) with no
headers (or invalid key) and assert_response :unauthorized and the expected
error payload, and two tests that call the same endpoints using
api_headers(created_api_key_with_no_holdings_scope) (or create_api_key(scopes:
[])) and assert_response :forbidden and the expected error payload; use the
existing helpers api_headers, create_api_key (or similar) and the urls
api_v1_holdings_url and api_v1_holding_url(`@holding`) so the new assertions
mirror the style of the existing tests (JSON.parse(response.body) and checking
"error" field).
In `@test/controllers/api/v1/trades_controller_test.rb`:
- Around line 32-85: Add missing write and auth behavioral tests to the trades
controller spec: create tests that attempt POST/PUT/PATCH/DELETE against
api_v1_trades_url and api_v1_trade_url covering successful create/update/destroy
flows and their error paths (invalid params → assert_response
:unprocessable_entity and invalid date → assert_response :unprocessable_entity),
a test asserting missing auth when no api_headers provided returns :unauthorized
(401), and a test asserting a read-only API key (use the same api_headers helper
but with a read-only key fixture) is blocked from write actions with :forbidden
(403). Reuse existing helpers and fixtures shown in this file (api_headers,
`@api_key`, create_trade, create_investment_account) and mirror the existing
patterns used in "lists trades scoped..." and "does not show a pending deletion
account trade" for parsing responses and asserting error payloads
("validation_failed", "not_found", etc.).
---
Outside diff comments:
In `@app/controllers/api/v1/accounts_controller.rb`:
- Around line 50-53: Change the 500 response in AccountsController so it follows
the project standard by returning the exception message instead of the static
string: replace the current render json block that sets message: "An unexpected
error occurred" with one that sets message: "Error: #{e.message}" (ensure this
is inside the rescue that has access to the exception variable `e` and update
the render call in AccountsController where the internal_server_error JSON is
returned).
- Around line 23-26: The 500 response in Api::V1::AccountsController currently
returns a generic message; update the render json call in the controller's
rescue/exception handling to use the project standard by setting message:
"Error: #{e.message}" (keeping error: "internal_server_error" and status:
:internal_server_error) and ensure the exception variable e is the one caught in
that rescue block.
In `@app/controllers/api/v1/holdings_controller.rb`:
- Around line 123-126: The 500 response in HoldingsController currently returns
a generic message; update the render in the rescue/exception handling block so
it follows the project pattern by using the exception message (e.g., render
json: { error: "internal_server_error", message: "Error: #{e.message}" },
status: :internal_server_error), ensuring you reference the caught exception
variable (e) in the rescue block and keep the same error key and status.
In `@app/controllers/api/v1/trades_controller.rb`:
- Around line 322-326: Replace the hardcoded 500 response message with the
project-standard error pattern so the rescue block in TradesController returns
message: "Error: #{e.message}"; locate the rescue/ensure handling around the
action methods in app/controllers/api/v1/trades_controller.rb (the block that
currently renders error: "internal_server_error") and update the JSON response
to use the exception object e to build the message string while keeping status:
:internal_server_error.
In `@app/controllers/api/v1/transactions_controller.rb`:
- Around line 195-198: The 500-response render in TransactionsController uses a
generic message; change the render json block (the hash passed to render json:
with keys error and message) to use the exception message pattern used across
API v1 by setting message to "Error: #{e.message}" (ensure this is inside the
rescue block where `e` is the caught exception, e.g., in the rescue in
TransactionsController action or rescue_from handler so `e` is in scope).
- Around line 172-175: The 500 response in the render block inside
TransactionsController currently returns a generic message; update the error
response to follow the project pattern by using the exception's message (i.e.,
set message to "Error: #{e.message}") in the render json call that returns
status: :internal_server_error so the controller (transactions_controller.rb)
exposes the actual error via the rescue e variable.
- Around line 51-54: Update the 500 error response in API v1 transactions
controller to follow the project pattern by returning the exception message;
locate the rescue block in app/controllers/api/v1/transactions_controller.rb
(the block that calls render json: { error: "internal_server_error", ... },
status: :internal_server_error) and change the message field to use the rescued
exception (message: "Error: #{e.message}") ensuring the rescue uses `rescue =>
e` so `e` is available.
- Around line 126-129: Replace the static 500 response message with the
project's standard error pattern by using the exception's message: in the
TransactionsController rescue block where you call render json: { error:
"internal_server_error", message: ... }, change the message to "Error:
#{e.message}" (keep the same status: :internal_server_error and the error key),
ensuring you reference the exception variable `e` used in that rescue so the
rendered JSON follows the API v1 standard.
- Around line 65-68: The 500 response in transactions_controller's error handler
should follow the project pattern by exposing the exception message; update the
render json block that currently returns message: "An unexpected error occurred"
to use message: "Error: #{e.message}" (ensure this is inside the rescue that
captures the exception as e) while keeping status: :internal_server_error and
the error key "internal_server_error".
In `@app/controllers/api/v1/valuations_controller.rb`:
- Around line 43-46: The 500 response in ValuationsController currently renders
a generic message; update the rescue block to render the project-standard
pattern by returning message: "Error: #{e.message}" (ensure the rescue uses
rescue => e so the exception variable e is available) while keeping error:
"internal_server_error" and status: :internal_server_error; locate the render
call in the controller (e.g., inside the rescue in the action or around method
handle) and replace the static message with the interpolated one.
- Around line 148-151: The 500 response in ValuationsController is returning a
hardcoded message instead of the project-standard error pattern; update the
rescue/render block (the code that currently does render json: { error:
"internal_server_error", message: "An unexpected error occurred" }, status:
:internal_server_error) to interpolate the exception message as message: "Error:
#{e.message}" so the response becomes { error: "internal_server_error", message:
"Error: #{e.message}" } while keeping status: :internal_server_error and using
the same rescue variable (e) from the surrounding rescue handler.
- Around line 55-58: The 500 response in the ValuationsController is using a
generic message; update the render call in the rescue/exception handling block
(where render json: { error: "internal_server_error", message: ... }, status:
:internal_server_error) to follow project convention by interpolating the
exception message (message: "Error: #{e.message}") so the JSON response exposes
the actual error string; locate the rescue block in ValuationsController (the
method handling the request or the around/rescue_from handler) and replace the
static "An unexpected error occurred" with the interpolated error message.
---
Nitpick comments:
In `@db/migrate/20260514090000_restore_trade_category_reference.rb`:
- Line 13: The migration's down method uses remove_reference :trades, :category,
foreign_key: true, type: :uuid but should mirror the up method's foreign key
options; update the remove_reference call to include the same on_delete option
so the foreign key spec matches (e.g., change foreign_key: true to foreign_key:
{ on_delete: :nullify }) for the remove_reference :trades, :category invocation
in the migration.
🪄 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: fc629522-3d1e-4dc8-9b21-bc16b9599c49
📒 Files selected for processing (39)
app/controllers/api/v1/accounts_controller.rbapp/controllers/api/v1/balance_sheet_controller.rbapp/controllers/api/v1/balances_controller.rbapp/controllers/api/v1/holdings_controller.rbapp/controllers/api/v1/trades_controller.rbapp/controllers/api/v1/transactions_controller.rbapp/controllers/api/v1/valuations_controller.rbapp/controllers/concerns/api/v1/security_resource_filtering.rbapp/controllers/concerns/api/v1/transfer_decision_filtering.rbapp/models/account.rbapp/models/balance_sheet.rbapp/models/balance_sheet/account_totals.rbapp/models/balance_sheet/net_worth_series_builder.rbapp/models/family.rbdb/migrate/20260514090000_restore_trade_category_reference.rbdb/schema.rbdocs/api/closed-account-portability.mddocs/api/openapi.yamlspec/requests/api/v1/accounts_spec.rbspec/requests/api/v1/balance_sheet_spec.rbspec/requests/api/v1/balances_spec.rbspec/requests/api/v1/holdings_spec.rbspec/requests/api/v1/rejected_transfers_spec.rbspec/requests/api/v1/securities_spec.rbspec/requests/api/v1/trades_spec.rbspec/requests/api/v1/transfers_spec.rbspec/requests/api/v1/valuations_spec.rbspec/swagger_helper.rbtest/controllers/api/v1/accounts_controller_test.rbtest/controllers/api/v1/balance_sheet_controller_test.rbtest/controllers/api/v1/balances_controller_test.rbtest/controllers/api/v1/holdings_controller_test.rbtest/controllers/api/v1/rejected_transfers_controller_test.rbtest/controllers/api/v1/securities_controller_test.rbtest/controllers/api/v1/trades_controller_test.rbtest/controllers/api/v1/transactions_controller_test.rbtest/controllers/api/v1/transfers_controller_test.rbtest/controllers/api/v1/valuations_controller_test.rbtest/models/balance_sheet_test.rb
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/controllers/api/v1/balance_sheet_controller.rb (1)
10-34: 💤 Low valueOptional: memoize
include_disabled_accounts?to avoid double-casting.The helper is invoked twice per request (once for
family.balance_sheetand once for the JSON response). Memoizing keeps both call sites consistent and avoids re-casting params.♻️ Suggested refactor
def show family = current_resource_owner.family + include_disabled = include_disabled_accounts? balance_sheet = family.balance_sheet( user: current_resource_owner, - include_disabled: include_disabled_accounts? + include_disabled: include_disabled ) render json: { currency: family.currency, - include_disabled: include_disabled_accounts?, + include_disabled: include_disabled, net_worth: balance_sheet.net_worth_money.as_json, assets: balance_sheet.assets.total_money.as_json, liabilities: balance_sheet.liabilities.total_money.as_json } end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/balance_sheet_controller.rb` around lines 10 - 34, Memoize include_disabled_accounts? so the param is cast only once: replace the current method implementation with an instance-variable-backed memo (e.g. set `@include_disabled_accounts` to the result of ActiveModel::Type::Boolean.new.cast(params[:include_disabled]) || false) and update usages (family.balance_sheet(..., include_disabled: include_disabled_accounts?) and the JSON render) to keep both call sites consistent and avoid double-casting; keep the method name include_disabled_accounts? and preserve returning a boolean.app/controllers/api/v1/transactions_controller.rb (1)
209-241: 💤 Low valueOptional: collapse the readable/writable transaction lookups into one helper.
The two methods share UUID validation, family/transaction lookup, entry assignment, and rescue handling — only the merged account scope differs. A small parameterization removes the duplication and keeps the two flows aligned.
♻️ Suggested refactor
- def set_readable_transaction - raise ActiveRecord::RecordNotFound unless valid_uuid?(params[:id]) - - family = current_resource_owner.family - `@transaction` = family.transactions - .joins(entry: :account) - .merge(Account.accessible_by(current_resource_owner)) - .merge(Account.historical) - .find(params[:id]) - `@entry` = `@transaction.entry` - rescue ActiveRecord::RecordNotFound - render json: { - error: "not_found", - message: "Transaction not found" - }, status: :not_found - end - - def set_writable_transaction - raise ActiveRecord::RecordNotFound unless valid_uuid?(params[:id]) - - family = current_resource_owner.family - `@transaction` = family.transactions - .joins(entry: :account) - .merge(Account.writable_by(current_resource_owner)) - .merge(Account.visible) - .find(params[:id]) - `@entry` = `@transaction.entry` - rescue ActiveRecord::RecordNotFound - render json: { - error: "not_found", - message: "Transaction not found" - }, status: :not_found - end + def set_readable_transaction + load_transaction!(Account.accessible_by(current_resource_owner), Account.historical) + end + + def set_writable_transaction + load_transaction!(Account.writable_by(current_resource_owner), Account.visible) + end + + def load_transaction!(*account_scopes) + raise ActiveRecord::RecordNotFound unless valid_uuid?(params[:id]) + + query = current_resource_owner.family.transactions.joins(entry: :account) + `@transaction` = account_scopes.reduce(query) { |q, scope| q.merge(scope) }.find(params[:id]) + `@entry` = `@transaction.entry` + rescue ActiveRecord::RecordNotFound + render json: { + error: "not_found", + message: "Transaction not found" + }, status: :not_found + end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/transactions_controller.rb` around lines 209 - 241, The two methods set_readable_transaction and set_writable_transaction are duplicated; extract a single helper (e.g., set_transaction_with_account_scope or set_transaction(scope)) that performs the UUID check (valid_uuid?(params[:id])), loads family = current_resource_owner.family, performs family.transactions.joins(entry: :account).merge(<account_scope>).find(params[:id]), assigns `@transaction` and `@entry`, and rescues ActiveRecord::RecordNotFound to render the same JSON; then replace set_readable_transaction to call the helper with Account.accessible_by(current_resource_owner).merge(Account.historical) and set_writable_transaction to call it with Account.writable_by(current_resource_owner).merge(Account.visible).app/controllers/api/v1/trades_controller.rb (1)
119-140: 💤 Low valueOptional: deduplicate
set_readable_tradeandset_writable_trade.The two helpers diverge only in the merged account scopes; the UUID guard, family lookup, entry assignment, and rescue branch are identical. Consider parameterizing the scopes through a shared loader to reduce maintenance surface (same pattern observation applies to
transactions_controller.rb).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/trades_controller.rb` around lines 119 - 140, Both set_readable_trade and set_writable_trade duplicate UUID validation, family lookup, entry assignment, and rescue behavior; extract a single loader that accepts an account_scope parameter and use it from both methods. Create a private method (e.g., load_trade_with_account_scope(account_scope)) that performs valid_uuid?(params[:id]), finds the trade via either trade_history_scope.find or family.trades.joins(entry: :account).merge(account_scope).find, sets `@trade` and `@entry`, and rescues ActiveRecord::RecordNotFound to render the same JSON error; then refactor set_readable_trade and set_writable_trade to call this loader with appropriate scopes (for writable use Account.writable_by(current_resource_owner).merge(Account.visible), for readable pass Account.visible or nil). Apply the same pattern to transactions_controller.rb where similar duplication exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/controllers/api/v1/balance_sheet_controller.rb`:
- Around line 10-34: Memoize include_disabled_accounts? so the param is cast
only once: replace the current method implementation with an
instance-variable-backed memo (e.g. set `@include_disabled_accounts` to the result
of ActiveModel::Type::Boolean.new.cast(params[:include_disabled]) || false) and
update usages (family.balance_sheet(..., include_disabled:
include_disabled_accounts?) and the JSON render) to keep both call sites
consistent and avoid double-casting; keep the method name
include_disabled_accounts? and preserve returning a boolean.
In `@app/controllers/api/v1/trades_controller.rb`:
- Around line 119-140: Both set_readable_trade and set_writable_trade duplicate
UUID validation, family lookup, entry assignment, and rescue behavior; extract a
single loader that accepts an account_scope parameter and use it from both
methods. Create a private method (e.g.,
load_trade_with_account_scope(account_scope)) that performs
valid_uuid?(params[:id]), finds the trade via either trade_history_scope.find or
family.trades.joins(entry: :account).merge(account_scope).find, sets `@trade` and
`@entry`, and rescues ActiveRecord::RecordNotFound to render the same JSON error;
then refactor set_readable_trade and set_writable_trade to call this loader with
appropriate scopes (for writable use
Account.writable_by(current_resource_owner).merge(Account.visible), for readable
pass Account.visible or nil). Apply the same pattern to
transactions_controller.rb where similar duplication exists.
In `@app/controllers/api/v1/transactions_controller.rb`:
- Around line 209-241: The two methods set_readable_transaction and
set_writable_transaction are duplicated; extract a single helper (e.g.,
set_transaction_with_account_scope or set_transaction(scope)) that performs the
UUID check (valid_uuid?(params[:id])), loads family =
current_resource_owner.family, performs family.transactions.joins(entry:
:account).merge(<account_scope>).find(params[:id]), assigns `@transaction` and
`@entry`, and rescues ActiveRecord::RecordNotFound to render the same JSON; then
replace set_readable_transaction to call the helper with
Account.accessible_by(current_resource_owner).merge(Account.historical) and
set_writable_transaction to call it with
Account.writable_by(current_resource_owner).merge(Account.visible).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f6176d7-2086-49b6-a206-1458141f6c60
📒 Files selected for processing (18)
app/controllers/api/v1/accounts_controller.rbapp/controllers/api/v1/balance_sheet_controller.rbapp/controllers/api/v1/holdings_controller.rbapp/controllers/api/v1/trades_controller.rbapp/controllers/api/v1/transactions_controller.rbapp/controllers/api/v1/valuations_controller.rbdb/migrate/20260514090000_restore_trade_category_reference.rbdb/schema.rbdocs/api/openapi.yamlspec/requests/api/v1/holdings_spec.rbspec/requests/api/v1/trades_spec.rbtest/controllers/api/v1/accounts_controller_test.rbtest/controllers/api/v1/balance_sheet_controller_test.rbtest/controllers/api/v1/holdings_controller_test.rbtest/controllers/api/v1/trades_controller_test.rbtest/controllers/api/v1/transactions_controller_test.rbtest/controllers/api/v1/valuations_controller_test.rbtest/models/trade_test.rb
✅ Files skipped from review due to trivial changes (1)
- db/schema.rb
🚧 Files skipped from review as they are similar to previous changes (5)
- test/controllers/api/v1/balance_sheet_controller_test.rb
- db/migrate/20260514090000_restore_trade_category_reference.rb
- app/controllers/api/v1/holdings_controller.rb
- spec/requests/api/v1/holdings_spec.rb
- spec/requests/api/v1/trades_spec.rb
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/controllers/api/v1/holdings_controller_test.rb (1)
32-45: ⚡ Quick winConsider adding explicit coverage for active account holdings.
The test focuses on disabled and pending_deletion accounts, which is great for verifying the new historical scoping behavior. However, since
@accountis created as active but immediately disabled (line 33), the test never explicitly verifies that active account holdings are included in the index.For completeness, consider either:
- Testing both active and disabled holdings in this test, or
- Adding a separate baseline test that verifies active account holdings are accessible
♻️ Proposed enhancement
test "lists holdings scoped to accessible historical accounts" do + active_holding = `@holding` # Keep reference to the active account holding + disabled_account = create_investment_account(status: "active", name: "Disabled Holding") + disabled_holding = create_holding(disabled_account, ticker: "DIS#{SecureRandom.hex(4).upcase}") + disabled_account.disable! - `@account.disable`! pending_deletion_account = create_investment_account(status: "pending_deletion", name: "Pending Delete Holding") pending_deletion_holding = create_holding(pending_deletion_account, ticker: "PDH#{SecureRandom.hex(4).upcase}") get api_v1_holdings_url, headers: api_headers(`@api_key`) assert_response :success response_data = JSON.parse(response.body) holding_ids = response_data["holdings"].map { |holding| holding["id"] } + assert_includes holding_ids, active_holding.id - assert_includes holding_ids, `@holding.id` + assert_includes holding_ids, disabled_holding.id assert_not_includes holding_ids, pending_deletion_holding.id assert_not_includes holding_ids, `@other_holding.id` end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/controllers/api/v1/holdings_controller_test.rb` around lines 32 - 45, The current test "lists holdings scoped to accessible historical accounts" disables `@account`, so it never asserts that holdings for an active account are returned; update the test to explicitly cover an active account by either not disabling `@account` (leave it active) and asserting `@holding.id` is included, or create a separate active_account and active_holding via create_investment_account and create_holding and assert their id appears in the response from get api_v1_holdings_url (use api_headers(`@api_key`)); keep the existing assertions for pending_deletion_holding and `@other_holding` unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/controllers/api/v1/holdings_controller_test.rb`:
- Around line 32-45: The current test "lists holdings scoped to accessible
historical accounts" disables `@account`, so it never asserts that holdings for an
active account are returned; update the test to explicitly cover an active
account by either not disabling `@account` (leave it active) and asserting
`@holding.id` is included, or create a separate active_account and active_holding
via create_investment_account and create_holding and assert their id appears in
the response from get api_v1_holdings_url (use api_headers(`@api_key`)); keep the
existing assertions for pending_deletion_holding and `@other_holding` unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4eb792e1-3ca8-40f9-9648-d16660686602
📒 Files selected for processing (4)
app/controllers/api/v1/balance_sheet_controller.rbapp/controllers/api/v1/trades_controller.rbapp/controllers/api/v1/transactions_controller.rbtest/controllers/api/v1/holdings_controller_test.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- app/controllers/api/v1/balance_sheet_controller.rb
- app/controllers/api/v1/transactions_controller.rb
- app/controllers/api/v1/trades_controller.rb
|
This is otherwise very clean and ready to merge. One issue to fix before landing: the API error rescue block exposes raw exception messages ( Generated by Claude Code |
Contributor
Author
This has been resolved as well. |
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
3 participants
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
What changed
pending_deletion.include_disabled=truesupport toGET /api/v1/balance_sheetwhile keeping the default response unchanged.trades.category_idschema support for the existing trades API category response and request contract.Why
Closed/disabled accounts are lifecycle state, not deleted history. Hiding them from normal account navigation should not remove their transactions, trades, holdings, valuations, balances, transfer decisions, or securities from API portability and reporting surfaces.
pending_deletionremains excluded because it represents destructive cleanup in progress.Validation
bin/rails test test/controllers/api/v1/accounts_controller_test.rb test/controllers/api/v1/transactions_controller_test.rb test/controllers/api/v1/balances_controller_test.rb test/controllers/api/v1/valuations_controller_test.rb test/controllers/api/v1/securities_controller_test.rb test/controllers/api/v1/transfers_controller_test.rb test/controllers/api/v1/rejected_transfers_controller_test.rb test/controllers/api/v1/balance_sheet_controller_test.rb test/controllers/api/v1/trades_controller_test.rb test/controllers/api/v1/holdings_controller_test.rb test/models/balance_sheet_test.rbRAILS_ENV=test bundle exec rake rswag:specs:swaggerizebin/rubocopbin/brakeman --no-pagerbin/rails zeitwerk:checkgit diff --checkcoderabbit review --agent -t uncommitted -c AGENTS.mdNotes
Summary by CodeRabbit
New Features
Improvements
Database
Documentation
Tests