Add per-account toggle to disable automatic transaction categorization#1700
Add per-account toggle to disable automatic transaction categorization#1700mike-lloyd03 wants to merge 1 commit intowe-promise:mainfrom
Conversation
brin-security-scanner
Bot
added
contributor:verified
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds a per-account boolean flag to enable/disable automatic category matching, including DB column and migration, a protected PATCH route and controller action to toggle it, strong-param whitelist, processor guard to skip matching when disabled, UI toggle and locales, and updated tests. ChangesCategory Matcher Toggle Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/concerns/accountable_resource.rb (1)
104-109:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConstrain
enable_category_matcherin shared strong paramsLine 108 permits this flag through generic account create/update params. That allows crafted requests to set it outside the linked-account toggle flow, which can produce unexpected behavior later (for example after linking a previously manual account). Prefer permitting this key only in
toggle_category_matcher(or conditionally when the target account is linked).🤖 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/concerns/accountable_resource.rb` around lines 104 - 109, The strong params block in params.require(:account).permit currently allows :enable_category_matcher unconditionally, letting callers set it outside the intended toggle flow; remove :enable_category_matcher from the generic permit list in AccountableResource (the params.require(:account).permit call) and instead allow it only in the dedicated toggle action (e.g., toggle_category_matcher) or conditionally when the account is linked (check account.linked? or equivalent) before permitting or applying the change; update the toggle_category_matcher controller method to accept and validate enable_category_matcher and ensure other create/update flows never accept that key.
🧹 Nitpick comments (1)
test/system/accounts_test.rb (1)
158-158: ⚡ Quick winPrefer a less brittle assertion than fixed heading level
Line 158 couples this flow test to
h3. Asserting text presence (or a stable test id) will reduce false failures from purely visual/semantic heading changes.Suggested tweak
- assert_selector "h3", text: "Updated account name" + assert_text "Updated account name"🤖 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/system/accounts_test.rb` at line 158, The test currently uses a brittle selector assert_selector "h3", text: "Updated account name"; replace it with a more stable assertion such as asserting the text exists (e.g., use assert_text "Updated account name") or target a stable test id/data attribute (e.g., assert_selector "[data-test='account-name']", text: "Updated account name") so the spec no longer depends on the heading level; update the assertion in the accounts_test flow that references "Updated account name" accordingly.
🤖 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/accounts_controller.rb`:
- Around line 98-100: The toggle_category_matcher action currently casts
params.dig(...) which treats a missing key as false; instead require and permit
the param so missing requests fail fast. Update toggle_category_matcher to pull
params via something like
params.require(:account).permit(:enable_category_matcher) (or use require +
fetch on :enable_category_matcher) and then cast the permitted value with
ActiveModel::Type::Boolean before calling `@account.update`! so malformed/partial
requests raise ParameterMissing rather than silently disabling the feature.
---
Outside diff comments:
In `@app/controllers/concerns/accountable_resource.rb`:
- Around line 104-109: The strong params block in
params.require(:account).permit currently allows :enable_category_matcher
unconditionally, letting callers set it outside the intended toggle flow; remove
:enable_category_matcher from the generic permit list in AccountableResource
(the params.require(:account).permit call) and instead allow it only in the
dedicated toggle action (e.g., toggle_category_matcher) or conditionally when
the account is linked (check account.linked? or equivalent) before permitting or
applying the change; update the toggle_category_matcher controller method to
accept and validate enable_category_matcher and ensure other create/update flows
never accept that key.
---
Nitpick comments:
In `@test/system/accounts_test.rb`:
- Line 158: The test currently uses a brittle selector assert_selector "h3",
text: "Updated account name"; replace it with a more stable assertion such as
asserting the text exists (e.g., use assert_text "Updated account name") or
target a stable test id/data attribute (e.g., assert_selector
"[data-test='account-name']", text: "Updated account name") so the spec no
longer depends on the heading level; update the assertion in the accounts_test
flow that references "Updated account name" 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: ddfd8dcc-8684-47a1-9947-883bc11f452f
📒 Files selected for processing (10)
app/controllers/accounts_controller.rbapp/controllers/concerns/accountable_resource.rbapp/models/plaid_entry/processor.rbapp/views/accounts/_form.html.erbconfig/locales/views/accounts/en.ymlconfig/routes.rbdb/migrate/20260429120000_add_enable_category_matcher_to_accounts.rbdb/schema.rbtest/models/plaid_entry/processor_test.rbtest/system/accounts_test.rb
| def toggle_category_matcher | ||
| value = ActiveModel::Type::Boolean.new.cast(params.dig(:account, :enable_category_matcher)) | ||
| @account.update!(enable_category_matcher: value) |
There was a problem hiding this comment.
Require the toggle param before casting to avoid accidental disable
At Line 99, cast(params.dig(...)) treats a missing key as false, so malformed/partial requests can silently disable category matching. Require + permit this field and fail fast when absent.
Proposed fix
def toggle_category_matcher
- value = ActiveModel::Type::Boolean.new.cast(params.dig(:account, :enable_category_matcher))
+ value = ActiveModel::Type::Boolean.new.cast(
+ params.require(:account).permit(:enable_category_matcher).fetch(:enable_category_matcher)
+ )
`@account.update`!(enable_category_matcher: value)
head :no_content
endAs per coding guidelines: app/controllers/**/*.rb: “Implement strong parameters and CSRF protection throughout the application”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def toggle_category_matcher | |
| value = ActiveModel::Type::Boolean.new.cast(params.dig(:account, :enable_category_matcher)) | |
| @account.update!(enable_category_matcher: value) | |
| def toggle_category_matcher | |
| value = ActiveModel::Type::Boolean.new.cast( | |
| params.require(:account).permit(:enable_category_matcher).fetch(:enable_category_matcher) | |
| ) | |
| `@account.update`!(enable_category_matcher: value) |
🤖 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/accounts_controller.rb` around lines 98 - 100, The
toggle_category_matcher action currently casts params.dig(...) which treats a
missing key as false; instead require and permit the param so missing requests
fail fast. Update toggle_category_matcher to pull params via something like
params.require(:account).permit(:enable_category_matcher) (or use require +
fetch on :enable_category_matcher) and then cast the permitted value with
ActiveModel::Type::Boolean before calling `@account.update`! so malformed/partial
requests raise ParameterMissing rather than silently disabling the feature.
Adds an `enable_category_matcher` boolean (default: true) to accounts so users can opt out of Plaid's automatic category suggestions on a per-account basis. When disabled, newly synced transactions arrive uncategorized so rules or manual assignment take precedence. - New migration adds `enable_category_matcher` column (default true, null: false) - `PlaidEntry::Processor#matched_category` gates the CategoryMatcher call on the account flag - Toggle rendered in the account edit modal for linked accounts (saves via main form submit) - `AccountableResource#account_params` permits the new field - `AccountsController#toggle_category_matcher` action added for potential API use - i18n strings added for label and hint text - Unit test covers the disabled-matcher path Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
mike-lloyd03
force-pushed
the
feature/per-account-category-matcher-toggle
branch
from
44d3a55 to
0badaa4
Compare
|
Solid, well-scoped feature — good backward-compatible default and clean guard placement. A few things worth clarifying: Turbo/response wiring for the toggle SimpleFIN parity h2 → h3 in system test YAML multi-line reformatting Reviewed by Claude Code Generated by Claude Code |
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Please fix broken tests and address PR review comments, @mike-lloyd03. 🙏
2 participants
This PR comes out of a discussion on Discord. I'd like to have transactions imported from Plaid not use the category matcher and just leave them uncategorized.
This PR (writted by Claude with me guiding and testing), adds a new toggle to disable this functionality on a per-account basis.
Summary
enable_category_matcherboolean column to accounts (defaulttrue, preserving existing behavior) so users can opt out of Plaid's automatic category suggestions on a per-account basisDetails
The Plaid integration maps
personal_finance_category.detailedto user-defined categories viaPlaidEntry::Processor. This is unconditional today — the initial category assignment can't be prevented even if the user prefers to categorize manually. A per-account toggle is more flexible than a per-family setting since users may want auto-categorization on one account but not another.The
enable_category_matchername is intentionally generic so it future-proofs for other providers (e.g. SimpleFIN) that may wire up category matching later.What is not changing:
Test plan
/accounts→ three-dot menu → Edit on a Plaid-linked account → confirm toggle renders defaulting to onbin/rails test test/models/plaid_entry/processor_test.rbpassesbin/rails testpassesbin/rubocop -f github -acleanbundle exec erb_lint ./app/**/*.erb -acleanbin/brakeman --no-pagerclean🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes