Add support to unlink lunch flow accounts #318
Conversation
WalkthroughAdds provider selection, linking, and unlinking flows across controllers, views, routes, models, locales, and tests; expands account linking to support both new account_providers and legacy Plaid/SimpleFIN associations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant AccountsController
participant ProviderSelector
Note over AccountsController,ProviderSelector: Select provider flow
User->>Browser: Click "Link provider"
Browser->>AccountsController: GET /accounts/:id/select_provider
AccountsController->>AccountsController: check account.linked?
alt not linked
AccountsController->>ProviderSelector: build `@available_providers`
ProviderSelector-->>AccountsController: list
AccountsController-->>Browser: Render select_provider (modal)
User->>Browser: Choose provider -> follow provider-specific link
else linked
AccountsController-->>Browser: Redirect with alert
end
sequenceDiagram
participant User
participant Browser
participant AccountsController
participant AccountProvider
Note over AccountsController,AccountProvider: Unlink flow
User->>Browser: Click "Unlink provider"
Browser->>AccountsController: GET /accounts/:id/confirm_unlink
AccountsController->>AccountsController: check account.linked?
alt linked
AccountsController-->>Browser: Render confirm_unlink (modal)
User->>Browser: Click "Delete"
Browser->>AccountsController: DELETE /accounts/:id/unlink
AccountsController->>AccountProvider: destroy_all for account
AccountsController->>AccountsController: clear legacy foreign keys (if present)
AccountProvider-->>AccountsController: destroyed
AccountsController-->>Browser: Redirect to /accounts with notice
else not linked
AccountsController-->>Browser: Redirect to account with alert
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![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
🧹 Nitpick comments (1)
test/controllers/accounts_controller_test.rb (1)
61-115: Good coverage! Consider testing multiple providers scenario.The tests comprehensively cover the main unlink flows. However, since the unlink action uses
destroy_allon account_providers, consider adding a test case for an account with multiple providers to ensure all associations are properly removed.Add this test case:
test "unlinks account with multiple providers" do plaid_account = plaid_accounts(:one) simplefin_account = simplefin_accounts(:one) AccountProvider.create!(account: @account, provider: plaid_account) AccountProvider.create!(account: @account, provider: simplefin_account) @account.reload assert_equal 2, @account.account_providers.count delete unlink_account_url(@account) @account.reload assert_equal 0, @account.account_providers.count assert_not @account.linked? end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/controllers/accounts_controller.rb(2 hunks)app/views/accounts/_account.html.erb(1 hunks)app/views/accounts/confirm_unlink.html.erb(1 hunks)config/locales/views/accounts/en.yml(2 hunks)config/routes.rb(1 hunks)test/controllers/accounts_controller_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (16)
config/**
📄 CodeRabbit inference engine (AGENTS.md)
Store application and environment configuration under config/
Files:
config/locales/views/accounts/en.ymlconfig/routes.rb
app/views/**/*.erb
📄 CodeRabbit inference engine (AGENTS.md)
app/views/**/*.erb: Keep heavy logic out of ERB views; prefer helpers/components instead
ERB templates are linted by erb-lint per .erb_lint.ymlAlways use the icon helper (icon) for icons; never call lucide_icon directly
Files:
app/views/accounts/confirm_unlink.html.erbapp/views/accounts/_account.html.erb
app/views/**/*.html.*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/views/**/*.html.*: Use partials only for simple, context-specific, mostly static HTML content.
Prefer semantic HTML; use Turbo Frames where possible instead of client-side solutions.
Use query params for state, not localStorage or sessionStorage.
Always perform server-side formatting for currencies, numbers, and dates.
Files:
app/views/accounts/confirm_unlink.html.erbapp/views/accounts/_account.html.erb
**/*.{html,erb,slim,js,jsx,ts,tsx,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Always use functional design tokens (e.g., text-primary, bg-container) from the design system; do not use raw colors or ad-hoc classes.
Files:
app/views/accounts/confirm_unlink.html.erbapp/views/accounts/_account.html.erb
app/views/**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
app/views/**/*.html.erb: Prefer native HTML elements (e.g., ,) over JS-based components
Leverage Turbo frames to break up pages instead of JS-driven client-side solutions
Prefer native client-side form validation when possible
Files:
app/views/accounts/confirm_unlink.html.erbapp/views/accounts/_account.html.erb
app/{models,controllers,views}/**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/views/accounts/confirm_unlink.html.erbapp/controllers/accounts_controller.rbapp/views/accounts/_account.html.erb
{app/javascript/controllers/**/*.{js,ts},app/views/**/*.erb,app/components/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Use declarative Stimulus actions in ERB (data-action) instead of imperative event listeners in controller lifecycle (e.g., avoid addEventListener in connect); controllers should just respond to actions
Files:
app/views/accounts/confirm_unlink.html.erbapp/views/accounts/_account.html.erb
{app/components/**/*.{js,ts,erb},app/views/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Component-scoped Stimulus controllers in app/components must be used only within their component views, not in app/views
Files:
app/views/accounts/confirm_unlink.html.erbapp/views/accounts/_account.html.erb
{app/views/**,app/helpers/**,app/javascript/controllers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
{app/views/**,app/helpers/**,app/javascript/controllers/**}: Style UI using TailwindCSS v4.x with the custom design system defined in app/assets/tailwind/maybe-design-system.css
Always start by referencing app/assets/tailwind/maybe-design-system.css to understand available primitives, functional tokens, and component tokens before styling
Prefer functional tokens from the design system over raw Tailwind values (e.g., use text-primary, bg-container, border border-primary instead of text-white, bg-white, border-gray-200)
Files:
app/views/accounts/confirm_unlink.html.erbapp/views/accounts/_account.html.erb
{app/views/**,app/helpers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
Always generate semantic HTML
Files:
app/views/accounts/confirm_unlink.html.erbapp/views/accounts/_account.html.erb
{app/views,app/components}/**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
{app/views,app/components}/**/*.html.erb: Keep domain logic out of ERB templates; compute values in component/controller code and reference them in the template
Integrate Stimulus declaratively in ERB: templates declare data-controller/data-action; controllers respond to those declarations
Pass data from Rails to Stimulus via data-*-value attributes, not inline JavaScript
Files:
app/views/accounts/confirm_unlink.html.erbapp/views/accounts/_account.html.erb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/
Files:
app/controllers/accounts_controller.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules
Files:
app/controllers/accounts_controller.rbconfig/routes.rbtest/controllers/accounts_controller_test.rb
**/*.{rb,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{rb,js,jsx,ts,tsx}: Make changes atomic, testable, and explain their impact briefly in code suggestions.
Respect existing tests and add tests when changing critical logic.
Files:
app/controllers/accounts_controller.rbconfig/routes.rbtest/controllers/accounts_controller_test.rb
app/views/**/_*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
Name partials with a leading underscore (e.g., _trend_change.html.erb, _form_errors.html.erb)
Files:
app/views/accounts/_account.html.erb
test/**/*_test.rb
📄 CodeRabbit inference engine (AGENTS.md)
Name Minitest files with *_test.rb and mirror the app/ structure under test/
test/**/*_test.rb: ALWAYS use Minitest + fixtures + Mocha for tests; NEVER RSpec or FactoryBot.
Use Mocha for mocking in tests when necessary.
Use VCR for external API tests.
test/**/*_test.rb: Always use Minitest for tests; do not use RSpec
Do not use factories (e.g., FactoryBot) in tests; rely on fixtures instead
For tests needing many records, use Rails helpers to construct data and inline the creation in the test
Only write tests for critical and important code paths
Avoid tests that merely verify framework/ActiveRecord behavior
Test boundaries correctly: for queries, assert returned values; for commands, assert collaborators are called with correct params
Never test the implementation details of one class in another class’s test suite
Use the mocha gem for stubs and mocks
Prefer OpenStruct for mock instances; use a mock class for complex cases
Only mock what’s necessary; don’t mock return values unless they are under test
Files:
test/controllers/accounts_controller_test.rb
🧠 Learnings (5)
📚 Learning: 2025-09-23T22:21:06.637Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-09-23T22:21:06.637Z
Learning: Applies to app/views/**/*.html.erb : Prefer native HTML elements (e.g., <dialog>, <details><summary>) over JS-based components
Applied to files:
app/views/accounts/confirm_unlink.html.erb
📚 Learning: 2025-09-23T22:24:15.106Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:24:15.106Z
Learning: Place context-specific partials within the relevant controller view directory (e.g., accounts/_account_sidebar_tabs.html.erb)
Applied to files:
app/views/accounts/confirm_unlink.html.erbapp/controllers/accounts_controller.rb
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/account.rb : Account syncs are orchestrated by Account#sync_data (auto-match transfers, compute balances from start_date..today, enrich transactions) and should run whenever an Entry is updated
Applied to files:
app/controllers/accounts_controller.rbapp/views/accounts/_account.html.erb
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/account/{balance_calculator.rb,balance/base_calculator.rb} : Calculate account balances daily using Account::BalanceCalculator and Account::Balance::BaseCalculator
Applied to files:
app/controllers/accounts_controller.rb
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/controllers/concerns/auto_sync.rb : Run daily Family syncs via the AutoSync concern; Family sync orchestrates Account and Plaid Item syncs
Applied to files:
app/controllers/accounts_controller.rb
🧬 Code graph analysis (2)
app/controllers/accounts_controller.rb (1)
app/models/account/linkable.rb (1)
linked?(14-16)
test/controllers/accounts_controller_test.rb (1)
app/models/account/linkable.rb (1)
linked?(14-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (6)
config/locales/views/accounts/en.yml (1)
6-6: LGTM!The translation keys are well-structured and provide clear, comprehensive messaging for the unlink flow. The warnings effectively communicate the implications of unlinking an account.
Also applies to: 89-97
app/views/accounts/_account.html.erb (1)
43-49: LGTM!The unlink option follows the existing pattern for the link option and correctly uses the icon helper with a turbo frame modal. The conditional logic ensures the link and unlink options are mutually exclusive.
config/routes.rb (1)
195-196: LGTM!The routes use appropriate HTTP methods (GET for confirmation, DELETE for unlink action) and are correctly defined as member routes.
app/views/accounts/confirm_unlink.html.erb (2)
6-6: Verify that Account#provider_name method exists.This view references
@account.provider_namewhich needs to exist on the Account model.See verification script in the controller review comment for app/controllers/accounts_controller.rb lines 81-94.
24-29: LGTM!The button component correctly uses the DS::Button component and properly sets up the DELETE request with turbo frame targeting.
app/controllers/accounts_controller.rb (1)
81-94: Account#provider_name is properly defined and unlink implementation is correct.Verification confirms:
- The
provider_namemethod exists in the Linkable module (included by Account) and correctly delegates toprovider&.provider_name- Post-unlink cleanup is handled automatically via
has_many :account_providers, dependent: :destroy— afterdestroy_all, thelinked?method returns false as expected- No additional sync or balance recalculation is required; the implementation is complete as-is
|
Expanded this to add support to link and unlink from ANY provider. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
test/controllers/simplefin_items_controller_test.rb (1)
239-245: Good edge case coverage.The test appropriately verifies the redirect behavior when no SimpleFIN accounts are available for linking.
Consider adding a complementary test for the happy path (when SimpleFIN accounts are available) to ensure the selection view renders correctly.
app/models/plaid_item.rb (1)
25-29: Extract duplicated accounts method to shared concern and add eager loading.This
accountsmethod is identical toSimplefinItem.accounts—both use the same pattern:map(&:current_account).compact.uniqto aggregate accounts across new and legacy systems.Additionally, both implementations lack eager loading. The
map(&:current_account)call will trigger queries per account sincecurrent_accountis a method returninglinked_account || account. SimplefinItem'sconnected_institutionsmethod demonstrates that.includes(:account)can be used to mitigate this.Extracting this logic to a shared concern would reduce duplication and allow both models to benefit from consistent eager-loading optimization in one place.
app/controllers/plaid_items_controller.rb (2)
51-66: Use i18n for user-facing messages.The alert on line 64 is hardcoded in English. For consistency with the project's i18n setup (locale files exist per the AI summary), extract this string to the locale file.
Apply this diff to use i18n:
if @available_plaid_accounts.empty? - redirect_to account_path(@account), alert: "No available Plaid accounts to link. Please connect a new Plaid account first." + redirect_to account_path(@account), alert: t(".no_available_accounts") endThen add to
config/locales/views/plaid_items/en.yml:plaid_items: select_existing_account: no_available_accounts: "No available Plaid accounts to link. Please connect a new Plaid account first."
Consider eager-loading optimization.
Lines 57-61 use
includes(:plaid_accounts)followed byflat_mapandselect, which still loads all plaid_accounts into memory before filtering. While functional, this could be optimized with a SQL-level query if the available accounts list grows large.
68-91: Use i18n for user-facing messages.Lines 74, 80, and 90 contain hardcoded English strings. Extract these to locale files for consistency.
Apply this diff:
unless Current.family.plaid_items.include?(plaid_account.plaid_item) - redirect_to account_path(@account), alert: "Invalid Plaid account selected" + redirect_to account_path(@account), alert: t(".invalid_account") return end if plaid_account.account_provider.present? || plaid_account.account.present? - redirect_to account_path(@account), alert: "This Plaid account is already linked" + redirect_to account_path(@account), alert: t(".already_linked") return end AccountProvider.create!( account: @account, provider: plaid_account ) - redirect_to accounts_path, notice: "Account successfully linked to Plaid" + redirect_to accounts_path, notice: t(".success")
Consider more efficient ownership verification.
Line 73 uses
.include?which loads all plaid_items into memory. For better performance with many items, consider a direct query.Replace lines 73-76 with:
unless Current.family.plaid_items.exists?(id: plaid_account.plaid_item_id) redirect_to account_path(@account), alert: t(".invalid_account") return end
Add error handling for AccountProvider creation.
Line 85 uses
create!which raises exceptions on validation failures. Consider wrapping in a rescue block or usingcreatewith error checking for more graceful failure handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/controllers/accounts_controller.rb(2 hunks)app/controllers/plaid_items_controller.rb(1 hunks)app/controllers/simplefin_items_controller.rb(1 hunks)app/models/account.rb(1 hunks)app/models/account/linkable.rb(2 hunks)app/models/plaid_item.rb(1 hunks)app/models/simplefin_item.rb(1 hunks)app/views/accounts/_account.html.erb(1 hunks)app/views/accounts/select_provider.html.erb(1 hunks)app/views/plaid_items/select_existing_account.html.erb(1 hunks)app/views/simplefin_items/select_existing_account.html.erb(1 hunks)config/locales/views/accounts/en.yml(2 hunks)config/locales/views/plaid_items/en.yml(1 hunks)config/locales/views/simplefin_items/en.yml(1 hunks)config/routes.rb(2 hunks)test/controllers/accounts_controller_test.rb(1 hunks)test/controllers/plaid_items_controller_test.rb(1 hunks)test/controllers/simplefin_items_controller_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (19)
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/
Files:
app/controllers/accounts_controller.rbapp/controllers/simplefin_items_controller.rbapp/models/simplefin_item.rbapp/controllers/plaid_items_controller.rbapp/models/plaid_item.rbapp/models/account.rbapp/models/account/linkable.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules
Files:
app/controllers/accounts_controller.rbapp/controllers/simplefin_items_controller.rbtest/controllers/simplefin_items_controller_test.rbapp/models/simplefin_item.rbapp/controllers/plaid_items_controller.rbapp/models/plaid_item.rbapp/models/account.rbapp/models/account/linkable.rbtest/controllers/accounts_controller_test.rbtest/controllers/plaid_items_controller_test.rbconfig/routes.rb
**/*.{rb,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{rb,js,jsx,ts,tsx}: Make changes atomic, testable, and explain their impact briefly in code suggestions.
Respect existing tests and add tests when changing critical logic.
Files:
app/controllers/accounts_controller.rbapp/controllers/simplefin_items_controller.rbtest/controllers/simplefin_items_controller_test.rbapp/models/simplefin_item.rbapp/controllers/plaid_items_controller.rbapp/models/plaid_item.rbapp/models/account.rbapp/models/account/linkable.rbtest/controllers/accounts_controller_test.rbtest/controllers/plaid_items_controller_test.rbconfig/routes.rb
app/{models,controllers,views}/**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/controllers/accounts_controller.rbapp/controllers/simplefin_items_controller.rbapp/views/simplefin_items/select_existing_account.html.erbapp/views/accounts/_account.html.erbapp/models/simplefin_item.rbapp/views/accounts/select_provider.html.erbapp/controllers/plaid_items_controller.rbapp/models/plaid_item.rbapp/models/account.rbapp/views/plaid_items/select_existing_account.html.erbapp/models/account/linkable.rb
app/views/**/*.erb
📄 CodeRabbit inference engine (AGENTS.md)
app/views/**/*.erb: Keep heavy logic out of ERB views; prefer helpers/components instead
ERB templates are linted by erb-lint per .erb_lint.ymlAlways use the icon helper (icon) for icons; never call lucide_icon directly
Files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/accounts/_account.html.erbapp/views/accounts/select_provider.html.erbapp/views/plaid_items/select_existing_account.html.erb
app/views/**/*.html.*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/views/**/*.html.*: Use partials only for simple, context-specific, mostly static HTML content.
Prefer semantic HTML; use Turbo Frames where possible instead of client-side solutions.
Use query params for state, not localStorage or sessionStorage.
Always perform server-side formatting for currencies, numbers, and dates.
Files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/accounts/_account.html.erbapp/views/accounts/select_provider.html.erbapp/views/plaid_items/select_existing_account.html.erb
**/*.{html,erb,slim,js,jsx,ts,tsx,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Always use functional design tokens (e.g., text-primary, bg-container) from the design system; do not use raw colors or ad-hoc classes.
Files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/accounts/_account.html.erbapp/views/accounts/select_provider.html.erbapp/views/plaid_items/select_existing_account.html.erb
app/views/**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
app/views/**/*.html.erb: Prefer native HTML elements (e.g., ,) over JS-based components
Leverage Turbo frames to break up pages instead of JS-driven client-side solutions
Prefer native client-side form validation when possible
Files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/accounts/_account.html.erbapp/views/accounts/select_provider.html.erbapp/views/plaid_items/select_existing_account.html.erb
{app/javascript/controllers/**/*.{js,ts},app/views/**/*.erb,app/components/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Use declarative Stimulus actions in ERB (data-action) instead of imperative event listeners in controller lifecycle (e.g., avoid addEventListener in connect); controllers should just respond to actions
Files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/accounts/_account.html.erbapp/views/accounts/select_provider.html.erbapp/views/plaid_items/select_existing_account.html.erb
{app/components/**/*.{js,ts,erb},app/views/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Component-scoped Stimulus controllers in app/components must be used only within their component views, not in app/views
Files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/accounts/_account.html.erbapp/views/accounts/select_provider.html.erbapp/views/plaid_items/select_existing_account.html.erb
{app/views/**,app/helpers/**,app/javascript/controllers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
{app/views/**,app/helpers/**,app/javascript/controllers/**}: Style UI using TailwindCSS v4.x with the custom design system defined in app/assets/tailwind/maybe-design-system.css
Always start by referencing app/assets/tailwind/maybe-design-system.css to understand available primitives, functional tokens, and component tokens before styling
Prefer functional tokens from the design system over raw Tailwind values (e.g., use text-primary, bg-container, border border-primary instead of text-white, bg-white, border-gray-200)
Files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/accounts/_account.html.erbapp/views/accounts/select_provider.html.erbapp/views/plaid_items/select_existing_account.html.erb
{app/views/**,app/helpers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
Always generate semantic HTML
Files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/accounts/_account.html.erbapp/views/accounts/select_provider.html.erbapp/views/plaid_items/select_existing_account.html.erb
{app/views,app/components}/**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
{app/views,app/components}/**/*.html.erb: Keep domain logic out of ERB templates; compute values in component/controller code and reference them in the template
Integrate Stimulus declaratively in ERB: templates declare data-controller/data-action; controllers respond to those declarations
Pass data from Rails to Stimulus via data-*-value attributes, not inline JavaScript
Files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/accounts/_account.html.erbapp/views/accounts/select_provider.html.erbapp/views/plaid_items/select_existing_account.html.erb
app/views/**/_*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
Name partials with a leading underscore (e.g., _trend_change.html.erb, _form_errors.html.erb)
Files:
app/views/accounts/_account.html.erb
config/**
📄 CodeRabbit inference engine (AGENTS.md)
Store application and environment configuration under config/
Files:
config/locales/views/simplefin_items/en.ymlconfig/locales/views/plaid_items/en.ymlconfig/locales/views/accounts/en.ymlconfig/routes.rb
test/**/*_test.rb
📄 CodeRabbit inference engine (AGENTS.md)
Name Minitest files with *_test.rb and mirror the app/ structure under test/
test/**/*_test.rb: ALWAYS use Minitest + fixtures + Mocha for tests; NEVER RSpec or FactoryBot.
Use Mocha for mocking in tests when necessary.
Use VCR for external API tests.
test/**/*_test.rb: Always use Minitest for tests; do not use RSpec
Do not use factories (e.g., FactoryBot) in tests; rely on fixtures instead
For tests needing many records, use Rails helpers to construct data and inline the creation in the test
Only write tests for critical and important code paths
Avoid tests that merely verify framework/ActiveRecord behavior
Test boundaries correctly: for queries, assert returned values; for commands, assert collaborators are called with correct params
Never test the implementation details of one class in another class’s test suite
Use the mocha gem for stubs and mocks
Prefer OpenStruct for mock instances; use a mock class for complex cases
Only mock what’s necessary; don’t mock return values unless they are under test
Files:
test/controllers/simplefin_items_controller_test.rbtest/controllers/accounts_controller_test.rbtest/controllers/plaid_items_controller_test.rb
app/models/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Domain models should not call Provider::Registry directly; use a Provided concern within the model’s namespace to select providers and expose convenience methods
Use ActiveRecord validations for forms and complex domain constraints.
app/models/**/*.rb: Place business logic in POROs and model classes under app/models
Models should answer questions about themselves (e.g., prefer account.balance_series over service objects)
Implement complex validations and business logic with ActiveRecord validations
Model-level validations may mirror DB constraints but are not strictly required
Files:
app/models/simplefin_item.rbapp/models/plaid_item.rbapp/models/account.rbapp/models/account/linkable.rb
app/{helpers,models}/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates on the server side (Ruby) before sending to the client
Files:
app/models/simplefin_item.rbapp/models/plaid_item.rbapp/models/account.rbapp/models/account/linkable.rb
app/models/account.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Account syncs are orchestrated by Account#sync_data (auto-match transfers, compute balances from start_date..today, enrich transactions) and should run whenever an Entry is updated
Files:
app/models/account.rb
🧠 Learnings (24)
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/account.rb : Account syncs are orchestrated by Account#sync_data (auto-match transfers, compute balances from start_date..today, enrich transactions) and should run whenever an Entry is updated
Applied to files:
app/controllers/accounts_controller.rbapp/models/simplefin_item.rbapp/models/account.rb
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/account/{balance_calculator.rb,balance/base_calculator.rb} : Calculate account balances daily using Account::BalanceCalculator and Account::Balance::BaseCalculator
Applied to files:
app/controllers/accounts_controller.rb
📚 Learning: 2025-09-23T22:24:15.106Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:24:15.106Z
Learning: Place context-specific partials within the relevant controller view directory (e.g., accounts/_account_sidebar_tabs.html.erb)
Applied to files:
app/controllers/accounts_controller.rbapp/views/simplefin_items/select_existing_account.html.erbapp/views/accounts/_account.html.erbapp/views/plaid_items/select_existing_account.html.erb
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/controllers/concerns/auto_sync.rb : Run daily Family syncs via the AutoSync concern; Family sync orchestrates Account and Plaid Item syncs
Applied to files:
app/controllers/accounts_controller.rbapp/controllers/plaid_items_controller.rb
📚 Learning: 2025-09-13T11:52:47.388Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/family/enable_banking_connectable.rb:27-30
Timestamp: 2025-09-13T11:52:47.388Z
Learning: Family connectable modules (like Family::PlaidConnectable and Family::EnableBankingConnectable) consistently use private methods that call Provider::Registry directly to access providers, rather than using Provided concerns.
Applied to files:
app/controllers/accounts_controller.rbapp/controllers/plaid_items_controller.rb
📚 Learning: 2025-09-13T11:27:55.271Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/controllers/enable_banking_items_controller.rb:48-62
Timestamp: 2025-09-13T11:27:55.271Z
Learning: The `create_enable_banking_item!` method in `Family::EnableBankingConnectable` uses `find_or_create_by(id: enable_banking_id)` to either find an existing EnableBankingItem or create a new one, so it automatically handles updates to existing items without creating duplicates.
Applied to files:
app/controllers/simplefin_items_controller.rbapp/controllers/plaid_items_controller.rb
📚 Learning: 2025-09-23T22:21:06.637Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-09-23T22:21:06.637Z
Learning: Applies to app/views/**/*.html.erb : Leverage Turbo frames to break up pages instead of JS-driven client-side solutions
Applied to files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/plaid_items/select_existing_account.html.erb
📚 Learning: 2025-09-23T22:21:06.637Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-09-23T22:21:06.637Z
Learning: Applies to app/views/**/*.html.erb : Prefer native HTML elements (e.g., <dialog>, <details><summary>) over JS-based components
Applied to files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/accounts/select_provider.html.erbapp/views/plaid_items/select_existing_account.html.erb
📚 Learning: 2025-09-23T22:21:06.637Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-09-23T22:21:06.637Z
Learning: Applies to app/views/layouts/application.html.erb : Use Turbo frames in the application layout to load controller actions as demonstrated
Applied to files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/plaid_items/select_existing_account.html.erb
📚 Learning: 2025-09-23T22:24:15.106Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:24:15.106Z
Learning: Applies to {app/views,app/components}/**/*.html.erb : Integrate Stimulus declaratively in ERB: templates declare data-controller/data-action; controllers respond to those declarations
Applied to files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/plaid_items/select_existing_account.html.erb
📚 Learning: 2025-09-23T22:23:22.963Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/{views,components}/**/*.html.erb : Integrate Stimulus declaratively in ERB: templates declare data-controller/actions/targets; controllers respond to those declarations
Applied to files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/plaid_items/select_existing_account.html.erb
📚 Learning: 2025-09-20T08:37:48.022Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T08:37:48.022Z
Learning: Applies to app/views/**/*.html.* : Prefer semantic HTML; use Turbo Frames where possible instead of client-side solutions.
Applied to files:
app/views/simplefin_items/select_existing_account.html.erb
📚 Learning: 2025-09-23T22:23:22.963Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/views/**/*.html.erb : Use ViewComponents when elements have complex logic/styling, are reused across contexts, need variants/slots/accessibility, or require Stimulus interactivity
Applied to files:
app/views/simplefin_items/select_existing_account.html.erb
📚 Learning: 2025-09-23T22:22:00.149Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/stimulus_conventions.mdc:0-0
Timestamp: 2025-09-23T22:22:00.149Z
Learning: Applies to app/{views,components}/**/*.html.erb : In ERB views, wire Stimulus interactions declaratively using data-action to call controller methods
Applied to files:
app/views/simplefin_items/select_existing_account.html.erbapp/views/plaid_items/select_existing_account.html.erb
📚 Learning: 2025-09-23T22:21:06.637Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-09-23T22:21:06.637Z
Learning: Applies to app/views/**/*.turbo_stream.erb : Use Turbo Streams to enhance functionality but avoid hard dependencies on them
Applied to files:
app/views/simplefin_items/select_existing_account.html.erb
📚 Learning: 2025-09-23T22:22:48.511Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-09-23T22:22:48.511Z
Learning: Applies to test/**/*_test.rb : Avoid tests that merely verify framework/ActiveRecord behavior
Applied to files:
test/controllers/simplefin_items_controller_test.rb
📚 Learning: 2025-09-13T11:40:26.913Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_entry/processor.rb:10-12
Timestamp: 2025-09-13T11:40:26.913Z
Learning: The plaid_id field is used consistently across multiple banking integrations (Plaid, SimpleFin, and Enable Banking) as a common external identifier field. A comprehensive refactor to use a proper external_id field should be done across all integrations in a separate PR to maintain consistency and avoid scope creep.
Applied to files:
app/controllers/plaid_items_controller.rbapp/models/plaid_item.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Applied to files:
app/models/plaid_item.rbapp/models/account.rbapp/models/account/linkable.rb
📚 Learning: 2025-09-13T11:33:54.159Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Applied to files:
app/models/account.rbapp/models/account/linkable.rb
📚 Learning: 2025-09-23T22:21:06.637Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-09-23T22:21:06.637Z
Learning: Applies to app/models/**/*.rb : Models should answer questions about themselves (e.g., prefer account.balance_series over service objects)
Applied to files:
app/models/account.rb
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/provider/concepts/**/*.rb : Define concept interfaces under app/models/provider/concepts (e.g., exchange_rate interface) to enable pluggable providers
Applied to files:
app/models/account/linkable.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Applied to files:
app/models/account/linkable.rb
📚 Learning: 2025-10-07T09:51:02.265Z
Learnt from: jjmata
Repo: we-promise/sure PR: 189
File: app/views/password_resets/new.html.erb:14-14
Timestamp: 2025-10-07T09:51:02.265Z
Learning: In the we-promise/sure repository, use i18n methods (like `t()` or `I18n.t`) for all user-facing strings to maintain proper internationalization infrastructure. While translations should be provided, extensive localization work is not a priority at this stage of development.
Applied to files:
config/locales/views/accounts/en.yml
📚 Learning: 2025-10-01T11:40:36.711Z
Learnt from: jjmata
Repo: we-promise/sure PR: 173
File: app/mailers/email_confirmation_mailer.rb:9-9
Timestamp: 2025-10-01T11:40:36.711Z
Learning: In the we-promise/sure repository, i18n (internationalization) is used throughout the application for translations. When making branding configurable (e.g., product names), it's appropriate to add parameters like `product: product_name` to existing `t()` translation calls rather than replacing them with hardcoded strings.
Applied to files:
config/locales/views/accounts/en.yml
🧬 Code graph analysis (8)
app/controllers/accounts_controller.rb (4)
app/models/account/linkable.rb (1)
linked?(14-16)app/models/family/simplefin_connectable.rb (1)
can_connect_simplefin?(8-10)app/models/family/plaid_connectable.rb (2)
can_connect_plaid_us?(8-10)can_connect_plaid_eu?(13-15)app/models/family/lunchflow_connectable.rb (1)
can_connect_lunchflow?(8-11)
app/controllers/simplefin_items_controller.rb (2)
app/controllers/plaid_items_controller.rb (2)
select_existing_account(51-66)link_existing_account(68-91)app/models/simplefin_item.rb (1)
accounts(28-31)
app/models/simplefin_item.rb (1)
app/models/plaid_item.rb (1)
accounts(26-29)
app/controllers/plaid_items_controller.rb (2)
app/controllers/simplefin_items_controller.rb (2)
select_existing_account(189-202)link_existing_account(204-227)app/models/plaid_item.rb (1)
accounts(26-29)
app/models/plaid_item.rb (2)
app/models/simplefin_item.rb (1)
accounts(28-31)app/models/plaid_item/accounts_snapshot.rb (1)
accounts(10-12)
app/models/account/linkable.rb (4)
app/models/provider/plaid_adapter.rb (1)
provider_name(50-52)app/models/provider/simplefin_adapter.rb (1)
provider_name(25-27)app/models/account_provider.rb (1)
provider_name(15-17)app/models/provider/base.rb (1)
provider_name(30-32)
test/controllers/accounts_controller_test.rb (1)
app/models/account/linkable.rb (1)
linked?(14-16)
test/controllers/plaid_items_controller_test.rb (1)
app/models/account/linkable.rb (1)
linked?(14-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (21)
config/locales/views/simplefin_items/en.yml (1)
48-52: LGTM! Translation keys are well-structured.The new translation block for
select_existing_accountfollows i18n best practices and provides all necessary keys for the linking UI flow.config/locales/views/plaid_items/en.yml (1)
27-31: LGTM! Consistent translation structure.The translation block mirrors the SimpleFIN implementation and provides proper i18n support for the Plaid linking flow.
app/models/simplefin_item.rb (1)
21-21: Good refactoring to distinguish legacy accounts.Renaming the association to
legacy_accountsclearly indicates these are accounts from the old system, improving code clarity.config/locales/views/accounts/en.yml (3)
6-7: LGTM! Generic provider labels improve flexibility.The addition of
link_providerandunlink_providerkeys appropriately generalizes the linking actions beyond the lunch flow-specific label.
90-98: Excellent user communication for the unlink flow.The
confirm_unlinktranslation block provides clear, comprehensive warnings about the consequences of unlinking. The structured warnings help users make informed decisions.
99-101: LGTM! Clear and concise provider selection labels.The
select_providertranslation keys provide appropriate context for the provider selection flow.app/views/accounts/select_provider.html.erb (2)
1-8: Excellent use of design system components and i18n.The dialog structure properly uses the DS::Dialog component and functional design tokens from the design system. Translation keys are correctly referenced.
9-22: Well-implemented provider selection UI.The implementation correctly:
- Uses functional design tokens (
border-primary,bg-container-hover,text-primary,text-secondary)- Leverages Turbo Frames for modal navigation
- Uses the
iconhelper as required by coding guidelines- Provides semantic HTML structure with proper hover states
test/controllers/plaid_items_controller_test.rb (2)
50-56: Good edge case coverage for Plaid accounts.The test properly verifies redirect behavior when no Plaid accounts are available, maintaining consistency with the SimpleFIN implementation.
58-76: Comprehensive test coverage for account linking.The test thoroughly verifies the linking behavior:
- Initial unlinked state
- AccountProvider creation
- Updated linked status
- Proper associations
This provides good coverage of the happy path for the linking flow.
app/models/plaid_item.rb (1)
19-19: Consistent refactoring with SimpleFIN.The renaming to
legacy_accountsmaintains consistency with the SimplefinItem model, improving code clarity across provider implementations.app/models/account.rb (1)
25-29: LGTM! Scope correctly filters manual accounts across both linking systems.The updated scope properly identifies manual (unlinked) accounts by checking both the new account_providers association and legacy foreign keys (plaid_account_id, simplefin_account_id). This aligns with the enhanced linked? logic in Account::Linkable.
test/controllers/accounts_controller_test.rb (1)
61-146: LGTM! Comprehensive test coverage for linking/unlinking flows.The test suite thoroughly covers the new provider linking and unlinking functionality:
- Tests both new (account_providers) and legacy (plaid_account_id) linking systems
- Verifies proper state transitions and validations
- Checks appropriate redirects and flash messages
- Confirms the deletion guard works correctly (linked accounts cannot be deleted until unlinked)
app/views/simplefin_items/select_existing_account.html.erb (1)
1-45: LGTM! Modal follows view conventions and design patterns.The view properly implements the SimpleFIN account selection flow:
- Uses DS::Dialog component with Turbo frame modal pattern
- Follows functional design token usage (text-primary, text-secondary, border-primary, etc.)
- Implements form with proper authenticity token and Turbo frame handling
- Consistent with the parallel Plaid items select_existing_account view
app/views/accounts/_account.html.erb (1)
36-50: LGTM! UI correctly implements unified link/unlink flows.The partial now properly uses:
account.linked?to check linkage status across both new and legacy systemsselect_provider_account_pathas the unified entry point for provider linking- Conditional rendering to show either "link" or "unlink" icons based on account state
- Account type filtering (Depository/CreditCard) for link eligibility
app/views/plaid_items/select_existing_account.html.erb (1)
1-45: LGTM! Consistent modal implementation for Plaid account selection.The view follows the same pattern as the SimpleFIN modal, maintaining consistency across provider linking flows. Properly uses DS components, Turbo frames, and functional design tokens.
app/models/account/linkable.rb (1)
13-16: LGTM! Correctly handles both new and legacy linking systems.The enhanced
linked?method properly checks all three linking mechanisms: new account_providers association and legacy plaid_account/simplefin_account foreign keys.app/controllers/accounts_controller.rb (2)
81-99: LGTM! Unlink action properly handles both linking systems.The unlink action correctly:
- Guards against unlinking already-unlinked accounts
- Destroys account_providers (new system)
- Clears plaid_account_id and simplefin_account_id (legacy system)
- Provides clear user feedback
101-152: LGTM! Provider selection logic is comprehensive.The select_provider action properly:
- Guards against linking already-linked accounts
- Checks family capabilities for each provider (SimpleFIN, Plaid US, Plaid EU, Lunchflow)
- Builds provider options with appropriate paths and descriptions
- Handles the edge case of no configured providers
app/controllers/simplefin_items_controller.rb (1)
189-227: LGTM! SimpleFIN linking actions follow established patterns.The select_existing_account and link_existing_account actions properly:
- Mirror the PlaidItemsController implementation for consistency
- Validate account ownership and linkage status before linking
- Create AccountProvider associations using the new linking system
- Handle error cases with appropriate redirects and alerts
config/routes.rb (1)
195-197: LGTM! Routes follow Rails conventions and align with the unlinking feature.The new routes are well-structured:
- Account member routes (lines 195-197) support the provider unlinking workflow with appropriate HTTP verbs
- PlaidItems and SimplefinItems collection routes (lines 283-297) follow a consistent pattern for linking existing accounts
- Route naming is clear and follows Rails conventions
Also applies to: 283-297
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/models/simplefin_item.rb (1)
97-100: Consider optimizing existence check.The
accounts.any?call loads all accounts into an array just to check if at least one exists. For items with many accounts, you could optimize this with an existence query instead:def has_completed_initial_setup? simplefin_accounts.joins(:account).exists? || simplefin_accounts.joins(:account_provider).exists? endHowever, given typical account counts per item, the current implementation is acceptable.
test/controllers/plaid_items_controller_test.rb (2)
84-89: Consider verifying reverse associations after linking.While the test correctly verifies that the Account becomes linked and has the expected AccountProvider count, it doesn't verify the reverse associations on the PlaidAccount. Since lines 74-75 assert these are nil before linking, consider asserting they're properly set afterward.
Apply this diff to add reverse association verification:
account.reload +plaid_account.reload assert account.linked?, "Account should be linked after creating AccountProvider" assert_equal 1, account.account_providers.count +assert_equal account, plaid_account.account, "PlaidAccount should reference the Account" +assert_not_nil plaid_account.account_provider, "PlaidAccount should have an AccountProvider" assert_redirected_to accounts_path assert_equal "Account successfully linked to Plaid", flash[:notice]
73-73: Lines 84–89: Add reverse association verification after linking.The test verifies the account is linked and has one account_provider, but doesn't verify the reverse: that
plaid_account.reload.accountandplaid_account.reload.account_providerare set. Adding these assertions would complete the boundary check that the relationship was established from both sides:assert_equal account, plaid_account.reload.account assert_not_nil plaid_account.reload.account_providerLine 73 is correct—the
accounts(:depository)fixture has no pre-existing associations and is unlinked.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/models/account/linkable.rb(2 hunks)app/models/plaid_item.rb(1 hunks)app/models/simplefin_item.rb(1 hunks)test/controllers/plaid_items_controller_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/account/linkable.rb
- app/models/plaid_item.rb
🧰 Additional context used
📓 Path-based instructions (7)
app/models/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Domain models should not call Provider::Registry directly; use a Provided concern within the model’s namespace to select providers and expose convenience methods
Use ActiveRecord validations for forms and complex domain constraints.
app/models/**/*.rb: Place business logic in POROs and model classes under app/models
Models should answer questions about themselves (e.g., prefer account.balance_series over service objects)
Implement complex validations and business logic with ActiveRecord validations
Model-level validations may mirror DB constraints but are not strictly required
Files:
app/models/simplefin_item.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/
Files:
app/models/simplefin_item.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules
Files:
app/models/simplefin_item.rbtest/controllers/plaid_items_controller_test.rb
**/*.{rb,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{rb,js,jsx,ts,tsx}: Make changes atomic, testable, and explain their impact briefly in code suggestions.
Respect existing tests and add tests when changing critical logic.
Files:
app/models/simplefin_item.rbtest/controllers/plaid_items_controller_test.rb
app/{helpers,models}/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates on the server side (Ruby) before sending to the client
Files:
app/models/simplefin_item.rb
app/{models,controllers,views}/**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/models/simplefin_item.rb
test/**/*_test.rb
📄 CodeRabbit inference engine (AGENTS.md)
Name Minitest files with *_test.rb and mirror the app/ structure under test/
test/**/*_test.rb: ALWAYS use Minitest + fixtures + Mocha for tests; NEVER RSpec or FactoryBot.
Use Mocha for mocking in tests when necessary.
Use VCR for external API tests.
test/**/*_test.rb: Always use Minitest for tests; do not use RSpec
Do not use factories (e.g., FactoryBot) in tests; rely on fixtures instead
For tests needing many records, use Rails helpers to construct data and inline the creation in the test
Only write tests for critical and important code paths
Avoid tests that merely verify framework/ActiveRecord behavior
Test boundaries correctly: for queries, assert returned values; for commands, assert collaborators are called with correct params
Never test the implementation details of one class in another class’s test suite
Use the mocha gem for stubs and mocks
Prefer OpenStruct for mock instances; use a mock class for complex cases
Only mock what’s necessary; don’t mock return values unless they are under test
Files:
test/controllers/plaid_items_controller_test.rb
🧠 Learnings (5)
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/account.rb : Account syncs are orchestrated by Account#sync_data (auto-match transfers, compute balances from start_date..today, enrich transactions) and should run whenever an Entry is updated
Applied to files:
app/models/simplefin_item.rb
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/{holding/base_calculator.rb,account/balance/base_calculator.rb} : For investment accounts, compute daily holdings via Holding::BaseCalculator and roll them into balances via Account::Balance::BaseCalculator
Applied to files:
app/models/simplefin_item.rb
📚 Learning: 2025-09-23T22:21:06.637Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-09-23T22:21:06.637Z
Learning: Applies to app/{models,controllers,views}/**/*.{rb,erb} : Avoid N+1 queries
Applied to files:
app/models/simplefin_item.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Applied to files:
app/models/simplefin_item.rb
📚 Learning: 2025-09-13T12:23:31.324Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
Applied to files:
app/models/simplefin_item.rb
🧬 Code graph analysis (2)
app/models/simplefin_item.rb (1)
app/models/plaid_item.rb (1)
accounts(26-33)
test/controllers/plaid_items_controller_test.rb (1)
app/models/account/linkable.rb (1)
linked?(14-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (2)
app/models/simplefin_item.rb (1)
27-35: N+1 query concern from previous review has been addressed.The preloading strategy correctly covers both association paths: the legacy
:accountassociation and the newaccount_provider: :accountpath. This prevents N+1 queries whencurrent_accountis called on eachsimplefin_account.The implementation is consistent with the PlaidItem pattern and more complete than the original suggestion (which only mentioned preloading
account_provider: :account).test/controllers/plaid_items_controller_test.rb (1)
50-56: No changes needed—fixture assumptions are valid.The test correctly relies on fixture state: only one PlaidAccount exists (plaid_accounts(:one)), and it's explicitly linked to accounts(:connected) via the legacy
accountforeign key. When the controller filters for unlinked accounts via.select { |pa| pa.account_provider.nil? && pa.account.nil? }, it correctly returns an empty list, causing the expected redirect. This fixture relationship is durable and not fragile.
|
This is great! I don't see the the method to delete the account once un-linked though. |
that's the nice thing, you don't need to. "Other accounts" can already be deleted, so it's part of the existing flow! |
Ahh, I found how to do it. |
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Tested locally, works nicely!
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
app/controllers/lunchflow_items_controller.rb (1)
281-283: Same name consistency consideration as line 101.See the previous comment for details.
🧹 Nitpick comments (2)
app/controllers/lunchflow_items_controller.rb (1)
100-102: Consider migrating existing record names for consistency.The
first_or_create!method only sets the name when creating a new record. ExistingLunchflowItemrecords will retain the old name "Lunchflow Connection" rather than being updated to "Lunch Flow Connection". While this is low impact (likely just a display name), you may want to run a data migration to update existing records for consistency.Example migration:
LunchflowItem.where(name: "Lunchflow Connection").update_all(name: "Lunch Flow Connection")This same consideration applies to line 282 in
link_existing_account.app/controllers/accounts_controller.rb (1)
107-147: Consider extracting provider configuration to reduce duplication.The pattern of checking provider availability and building provider hashes is repeated four times with similar structure. This could be refactored into a more maintainable approach.
Consider extracting the provider configuration into a method or constant:
def select_provider if @account.linked? redirect_to account_path(@account), alert: t(".already_linked") return end @available_providers = build_available_providers if @available_providers.empty? redirect_to account_path(@account), alert: t(".no_providers") return end end private def build_available_providers PROVIDER_CONFIGS.filter_map do |config| next unless family.send(config[:capability_method]) { name: t(".providers.#{config[:key]}.name"), key: config[:key], description: t(".providers.#{config[:key]}.description"), path: send(config[:path_helper], account_id: @account.id, **config[:path_params]) } end end PROVIDER_CONFIGS = [ { key: "simplefin", capability_method: :can_connect_simplefin?, path_helper: :select_existing_account_simplefin_items_path, path_params: {} }, { key: "plaid_us", capability_method: :can_connect_plaid_us?, path_helper: :select_existing_account_plaid_items_path, path_params: { region: "us" } }, { key: "plaid_eu", capability_method: :can_connect_plaid_eu?, path_helper: :select_existing_account_plaid_items_path, path_params: { region: "eu" } }, { key: "lunchflow", capability_method: :can_connect_lunchflow?, path_helper: :select_existing_account_lunchflow_items_path, path_params: {} } ].freeze
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/accounts_controller.rb(2 hunks)app/controllers/lunchflow_items_controller.rb(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-13T11:52:47.388Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/family/enable_banking_connectable.rb:27-30
Timestamp: 2025-09-13T11:52:47.388Z
Learning: Family connectable modules (like Family::PlaidConnectable and Family::EnableBankingConnectable) consistently use private methods that call Provider::Registry directly to access providers, rather than using Provided concerns.
Applied to files:
app/controllers/accounts_controller.rb
📚 Learning: 2025-09-13T11:27:55.271Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/controllers/enable_banking_items_controller.rb:48-62
Timestamp: 2025-09-13T11:27:55.271Z
Learning: The `create_enable_banking_item!` method in `Family::EnableBankingConnectable` uses `find_or_create_by(id: enable_banking_id)` to either find an existing EnableBankingItem or create a new one, so it automatically handles updates to existing items without creating duplicates.
Applied to files:
app/controllers/lunchflow_items_controller.rb
🧬 Code graph analysis (1)
app/controllers/accounts_controller.rb (4)
app/models/account/linkable.rb (1)
linked?(14-16)app/models/family/simplefin_connectable.rb (1)
can_connect_simplefin?(8-10)app/models/family/plaid_connectable.rb (2)
can_connect_plaid_us?(8-10)can_connect_plaid_eu?(13-15)app/models/family/lunchflow_connectable.rb (1)
can_connect_lunchflow?(8-11)
🔇 Additional comments (2)
app/controllers/lunchflow_items_controller.rb (1)
340-341: LGTM!The explicit name assignment ensures new items created through the UI will consistently use the updated display name.
app/controllers/accounts_controller.rb (1)
2-2: LGTM!The before_action update correctly includes all three new actions that require
@accountto be set.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/controllers/accounts_controller_test.rb (2)
33-33: Consider making the flash message assertion more dynamic.The flash message now includes the account type ("Depository account"). Hard-coding "Depository account" in the assertion makes the test fragile—if the fixture's account type changes, the test will fail even though the behavior is correct.
Apply this diff to make the assertion dynamic:
- assert_equal "Depository account scheduled for deletion", flash[:notice] + assert_equal "#{@account.accountable.name} scheduled for deletion", flash[:notice]Apply the same change on line 130.
Also applies to: 130-130
138-145: Expand test coverage to include legacy system checks.The test verifies that
select_providerredirects for accounts linked via the new system (AccountProvider), but doesn't test whether it also redirects for accounts linked via legacy associations (plaid_account_idorsimplefin_account_id). Sincelinked?checks all three, the redirect logic should be tested for all scenarios.Add these test cases after line 145:
test "select_provider redirects for account linked via legacy Plaid" do plaid_account = plaid_accounts(:one) @account.update!(plaid_account_id: plaid_account.id) get select_provider_account_url(@account) assert_redirected_to account_url(@account) assert_equal "Account is already linked to a provider", flash[:alert] end test "select_provider redirects for account linked via legacy SimpleFIN" do simplefin_account = simplefin_accounts(:one) @account.update!(simplefin_account_id: simplefin_account.id) get select_provider_account_url(@account) assert_redirected_to account_url(@account) assert_equal "Account is already linked to a provider", flash[:alert] end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/controllers/accounts_controller_test.rb(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/family/enable_banking_connectable.rb:27-30
Timestamp: 2025-09-13T11:52:47.388Z
Learning: Family connectable modules (like Family::PlaidConnectable and Family::EnableBankingConnectable) consistently use private methods that call Provider::Registry directly to access providers, rather than using Provided concerns.
🧬 Code graph analysis (1)
test/controllers/accounts_controller_test.rb (1)
app/models/account/linkable.rb (1)
linked?(14-16)
🔇 Additional comments (1)
test/controllers/accounts_controller_test.rb (1)
112-131: Good integration test for the unlink-then-delete flow.This test effectively validates the complete user journey: attempting to delete a linked account (which should fail), unlinking the account, and then successfully deleting it. While it tests multiple scenarios, this integration coverage is valuable for ensuring the full workflow behaves correctly.
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Working nicely! Unlinked and-relinked an account to test, locally.
…nify \"manual\" scope, and correct unlinked counts - Preserve #manual-accounts wrapper: switch non-empty updates to turbo_stream.update and background broadcast_update_to; keep empty-path replace to render <div id=\"manual-accounts\"></div> - Unify definition of manual accounts via Account.visible_manual (visible + legacy-nil + no AccountProvider); reuse in controllers, jobs, and helper - Correct setup/unlinked counts: SimplefinItem::Syncer#finalize_setup_counts and maps now consider AccountProvider links (legacy account AND provider must be absent) Deleted: - app/models/simplefin_item/relink_service.rb - app/controllers/concerns/simplefin_items/relink_helpers.rb - app/javascript/controllers/auto_relink_controller.js - app/views/simplefin_items/_relink_modal.html.erb - app/views/simplefin_items/manual_relink.html.erb - app/views/simplefin_items/relink.html.erb - test/services/simplefin_item/relink_service_test.rb Refs: PR we-promise#318 unified link/unlink; PR we-promise#267 SimpleFIN; follow-up to fix wrapper ID loss and counting drift."
* Add support to unlink lunch flow accounts * add support to link and unlink to any provider * Fix tests and query * Let's keep Amr happy about his brand * Wrap unlink operations in a transaction and add error handling. * Fix tests --------- Co-authored-by: Juan José Mata <[email protected]>
* SimpleFin: metadata + merge fixes; holdings (incl. crypto) + Day Change; Sync Summary; ops rakes; lint # Conflicts: # db/schema.rb # Conflicts: # app/controllers/simplefin_items_controller.rb * fix testing * fix linting * xfix linting x2 * Review PR #267 on we-promise/sure (SimpleFin enhancements v2). Address all 15 actionable CodeRabbit comments: Add UUID validations in rakes (e.g., simplefin_unlink), swap Ruby pattern matching/loops for efficient DB queries (e.g., where LOWER(name) LIKE ?), generate docstrings for low-coverage areas (31%), consolidate routes for simplefin_items, move view logic to helpers (e.g., format_transaction_extra), strengthen tests with exact assertions/fixtures for dedup/relink failures. Also, check for overlaps with merged #262 (merchants fix): Ensure merchant creation in simplefin_entry/processor.rb aligns with new payee-based flow and MD5 IDs; add tests for edge cases like empty payees or over-merging pendings. Prioritize security (PII redaction in logs, no hardcoded secrets). * SimpleFin: address CodeRabbit comments (batch 1) - Consolidate simplefin_items routes under a single resources block; keep URLs stable - Replace inline JS with Stimulus auto-relink controller; auto-load relink modal via global modal frame - Improve a11y in relink modal by wrapping rows in labels - Harden unlink rake: default dry_run=true, UUID validation, redact PII in outputs, clearer errors - Backfill rake: default dry_run=true, UUID validation; groundwork for per-SFA counters - Fix-was-merged rake: default dry_run=true, UUID validation; clearer outputs - Idempotent transfer auto-match (find_or_create_by! + RecordNotUnique rescue) - Extract SimpleFin error tooltip assembly into helper and use it in view RuboCop: maintain 2-space indentation, spaces inside array brackets, spaces after commas, and no redundant returns * Linter noise * removed filed commited by mistake. * manual relink flow and tighten composite matching * enforce manual relink UI; fix adapter keywords; guarantee extra.simplefin hash * refactor(simplefin): extract relink service; enforce manual relink UI; tighten composite match; migration 7.2 * add provider date parser; refactor rake; move view queries; partial resilience * run balances-only import in background job. make update flow enqueue balances-only job * persists across all update redirects and initialize used_manual_ids to prevent NameError in relink candidate computation. * SimpleFin: metadata + merge fixes; holdings (incl. crypto) + Day Change; Sync Summary; ops rakes; lint * Fixed failed test after rebase. * scan_ruby fix * Calming the rabbit: Fix AccountProvider linking when accounts change Drop the legacy unique index instead of duplicating it Fix dynamic constant assignment Use fixtures consistently; avoid rescue for control flow. Replace bare rescue with explicit exception class. Move business logic out of the view. Critical: Transaction boundary excludes recompute phase, risking data loss. Inconsistency between documentation and implementation for zero-error case. Refactor to use the compute_unlinked_count helper for consistency. Fix cleanup task default: it deletes by default. Move sync stats computation to controller to avoid N+1 queries. Consolidate duplicate sync query. Clarify the intent of setting flash notice on the error path. Fix Date/Time comparison in should_be_inactive?. Move stats retrieval logic to controller. Remove duplicate Sync summary section. Remove the unnecessary sleep statement; use Capybara's built-in waiting. Add label wrappers for accessibility and consistency. * FIX SimpleFIN new account modal Now new account properly loads as a Modal, instead of new page. Fixes also form showing dashboard instead of settings page. * Remove SimpleFin legacy UI components, migrate schema, and refine linking behavior. # Conflicts: # app/helpers/settings_helper.rb * Extract SimpleFin-related logic to `prepare_show_context` helper and refactor for consistency. Adjust conditional checks and ensure controller variables are properly initialized. * Remove unused SimpleFin maps from prepare_show_context; select IDs to avoid N+1 Replace Tailwind bg-green-500 with semantic bg-success in _simplefin_panel/_provider_form Add f.label :setup_token in simplefin_items/new for a11y Remove duplicate require in AccountsControllerSimplefinCtaTest * Remove unnecessary blank lines * Reduce unnecessary changes This reduces the diff against main * Simplefin Account Setup: Display in modal This fixes an issue with the `X` dismiss button in the top right corner * Removed unnecessary comment. * removed unnecessary function. * fixed broken links * Removed unnecessary file * changed to database query * set to use UTC and gaurd against null * set dry_run=true * Fixed comment * Changed to use a database-level query * matched test name to test behavior. * Eliminate code duplication and Time.zone dependency * make final summary surface failures * lint fix * Revised timezone comment. better handle missing selectors. * sanitized LIKE wildcards * Fixed SimpleFin import to avoid “Currency can’t be blank” validation failures when providers return an empty currency string. * Added helper methods for admin and self-hosted checks * Specify exception types in rescue clauses. * Refined logic to determine transaction dates for credit accounts. * Refined stats calculation for `total_accounts` to track the maximum unique accounts per run instead of accumulating totals. * Moved `unlink_all!` logic to `SimplefinItem::Unlinking` concern and deprecated `SimplefinItem::Unlinker`. Updated related references. * Refined legacy unlinking logic, improved `current_holdings` formatting, and added ENV-based overrides for self-hosted checks. * Enhanced `unlink_all!` with explicit error handling, improved transaction safety, and refined ENV-based self-hosted checks. Adjusted exception types and cleaned up private method handling. * Improved currency assignment logic by adding fallback to `current_account` and `family` currencies. * Enhanced error tracking during SimpleFin account imports by adding categorized error buckets, limiting stored errors to the last 5, and improving `stats` calculations. * typo fix * Didn't realize rabbit was still mad... Refactored SimpleFin error handling and CTA logic: centralized duplicate detection and relink visibility into controller, improved task counters, adjusted redirect notices, and fixed form indexing. * Dang rabbit never stops... Centralized SimpleFin maps logic into `MapsHelper` concern and integrated it into relevant controllers and rake tasks. Optimized queries, reduced redundancy, and improved unlinked counts and manual account checks with batch processing. Adjusted task arguments for clarity. * Persistent rabbit. Optimized SimpleFin maps logic by implementing batch queries for manual account and unlinked count checks, reducing N+1 issues. Improved clarity of rake task argument descriptions and error messages for better usability. * Lost a commit somehow, resolved here. Refactored transaction extra details logic by introducing `build_transaction_extra_details` helper to improve clarity, reusability, and reduce view complexity. Enhanced rake tasks with strict dry-run validation and better error handling. Updated schema to allow nullable `merchant_id` and added conditional unique indexes for recurring transactions. * Refactored sensitive data redaction in `simplefin_unlink` task for recursive handling, optimized SQL sanitization in `simplefin_holdings_backfill`, improved error handling in `transactions_helper`, and streamlined day change calculation logic in `Holding` model. * Lint fix * Removed per PR comments. * Also removing per PR comment. * git commit -m "SimpleFIN polish: preserve #manual-accounts wrapper, unify \"manual\" scope, and correct unlinked counts - Preserve #manual-accounts wrapper: switch non-empty updates to turbo_stream.update and background broadcast_update_to; keep empty-path replace to render <div id=\"manual-accounts\"></div> - Unify definition of manual accounts via Account.visible_manual (visible + legacy-nil + no AccountProvider); reuse in controllers, jobs, and helper - Correct setup/unlinked counts: SimplefinItem::Syncer#finalize_setup_counts and maps now consider AccountProvider links (legacy account AND provider must be absent) Deleted: - app/models/simplefin_item/relink_service.rb - app/controllers/concerns/simplefin_items/relink_helpers.rb - app/javascript/controllers/auto_relink_controller.js - app/views/simplefin_items/_relink_modal.html.erb - app/views/simplefin_items/manual_relink.html.erb - app/views/simplefin_items/relink.html.erb - test/services/simplefin_item/relink_service_test.rb Refs: PR #318 unified link/unlink; PR #267 SimpleFIN; follow-up to fix wrapper ID loss and counting drift." * Extend unlinked account check to include "Investment" type * set SimpleFIN item for `balances`, remove redundant unpacking, and improve holdings task error * SimpleFIN: add `errors` action + modal; do not reintroduce legacy relink actions; removed dead helper * FIX simpleFIN linking * Add delay back, tests benefit from it * Put cache back in * Remove empty `rake` task * Small spelling fixes. --------- Signed-off-by: soky srm <[email protected]> Co-authored-by: Josh Waldrep <[email protected]> Co-authored-by: Juan José Mata <[email protected]> Co-authored-by: sokie <[email protected]> Co-authored-by: Dylan Corrales <[email protected]>
* Add support to unlink lunch flow accounts * add support to link and unlink to any provider * Fix tests and query * Let's keep Amr happy about his brand * Wrap unlink operations in a transaction and add error handling. * Fix tests --------- Co-authored-by: Juan José Mata <[email protected]>
* Add support to unlink lunch flow accounts * add support to link and unlink to any provider * Fix tests and query * Let's keep Amr happy about his brand * Wrap unlink operations in a transaction and add error handling. * Fix tests --------- Co-authored-by: Juan José Mata <[email protected]>
* SimpleFin: metadata + merge fixes; holdings (incl. crypto) + Day Change; Sync Summary; ops rakes; lint # Conflicts: # db/schema.rb # Conflicts: # app/controllers/simplefin_items_controller.rb * fix testing * fix linting * xfix linting x2 * Review PR #267 on we-promise/sure (SimpleFin enhancements v2). Address all 15 actionable CodeRabbit comments: Add UUID validations in rakes (e.g., simplefin_unlink), swap Ruby pattern matching/loops for efficient DB queries (e.g., where LOWER(name) LIKE ?), generate docstrings for low-coverage areas (31%), consolidate routes for simplefin_items, move view logic to helpers (e.g., format_transaction_extra), strengthen tests with exact assertions/fixtures for dedup/relink failures. Also, check for overlaps with merged #262 (merchants fix): Ensure merchant creation in simplefin_entry/processor.rb aligns with new payee-based flow and MD5 IDs; add tests for edge cases like empty payees or over-merging pendings. Prioritize security (PII redaction in logs, no hardcoded secrets). * SimpleFin: address CodeRabbit comments (batch 1) - Consolidate simplefin_items routes under a single resources block; keep URLs stable - Replace inline JS with Stimulus auto-relink controller; auto-load relink modal via global modal frame - Improve a11y in relink modal by wrapping rows in labels - Harden unlink rake: default dry_run=true, UUID validation, redact PII in outputs, clearer errors - Backfill rake: default dry_run=true, UUID validation; groundwork for per-SFA counters - Fix-was-merged rake: default dry_run=true, UUID validation; clearer outputs - Idempotent transfer auto-match (find_or_create_by! + RecordNotUnique rescue) - Extract SimpleFin error tooltip assembly into helper and use it in view RuboCop: maintain 2-space indentation, spaces inside array brackets, spaces after commas, and no redundant returns * Linter noise * removed filed commited by mistake. * manual relink flow and tighten composite matching * enforce manual relink UI; fix adapter keywords; guarantee extra.simplefin hash * refactor(simplefin): extract relink service; enforce manual relink UI; tighten composite match; migration 7.2 * add provider date parser; refactor rake; move view queries; partial resilience * run balances-only import in background job. make update flow enqueue balances-only job * persists across all update redirects and initialize used_manual_ids to prevent NameError in relink candidate computation. * SimpleFin: metadata + merge fixes; holdings (incl. crypto) + Day Change; Sync Summary; ops rakes; lint * Fixed failed test after rebase. * scan_ruby fix * Calming the rabbit: Fix AccountProvider linking when accounts change Drop the legacy unique index instead of duplicating it Fix dynamic constant assignment Use fixtures consistently; avoid rescue for control flow. Replace bare rescue with explicit exception class. Move business logic out of the view. Critical: Transaction boundary excludes recompute phase, risking data loss. Inconsistency between documentation and implementation for zero-error case. Refactor to use the compute_unlinked_count helper for consistency. Fix cleanup task default: it deletes by default. Move sync stats computation to controller to avoid N+1 queries. Consolidate duplicate sync query. Clarify the intent of setting flash notice on the error path. Fix Date/Time comparison in should_be_inactive?. Move stats retrieval logic to controller. Remove duplicate Sync summary section. Remove the unnecessary sleep statement; use Capybara's built-in waiting. Add label wrappers for accessibility and consistency. * FIX SimpleFIN new account modal Now new account properly loads as a Modal, instead of new page. Fixes also form showing dashboard instead of settings page. * Remove SimpleFin legacy UI components, migrate schema, and refine linking behavior. # Conflicts: # app/helpers/settings_helper.rb * Extract SimpleFin-related logic to `prepare_show_context` helper and refactor for consistency. Adjust conditional checks and ensure controller variables are properly initialized. * Remove unused SimpleFin maps from prepare_show_context; select IDs to avoid N+1 Replace Tailwind bg-green-500 with semantic bg-success in _simplefin_panel/_provider_form Add f.label :setup_token in simplefin_items/new for a11y Remove duplicate require in AccountsControllerSimplefinCtaTest * Remove unnecessary blank lines * Reduce unnecessary changes This reduces the diff against main * Simplefin Account Setup: Display in modal This fixes an issue with the `X` dismiss button in the top right corner * Removed unnecessary comment. * removed unnecessary function. * fixed broken links * Removed unnecessary file * changed to database query * set to use UTC and gaurd against null * set dry_run=true * Fixed comment * Changed to use a database-level query * matched test name to test behavior. * Eliminate code duplication and Time.zone dependency * make final summary surface failures * lint fix * Revised timezone comment. better handle missing selectors. * sanitized LIKE wildcards * Fixed SimpleFin import to avoid “Currency can’t be blank” validation failures when providers return an empty currency string. * Added helper methods for admin and self-hosted checks * Specify exception types in rescue clauses. * Refined logic to determine transaction dates for credit accounts. * Refined stats calculation for `total_accounts` to track the maximum unique accounts per run instead of accumulating totals. * Moved `unlink_all!` logic to `SimplefinItem::Unlinking` concern and deprecated `SimplefinItem::Unlinker`. Updated related references. * Refined legacy unlinking logic, improved `current_holdings` formatting, and added ENV-based overrides for self-hosted checks. * Enhanced `unlink_all!` with explicit error handling, improved transaction safety, and refined ENV-based self-hosted checks. Adjusted exception types and cleaned up private method handling. * Improved currency assignment logic by adding fallback to `current_account` and `family` currencies. * Enhanced error tracking during SimpleFin account imports by adding categorized error buckets, limiting stored errors to the last 5, and improving `stats` calculations. * typo fix * Didn't realize rabbit was still mad... Refactored SimpleFin error handling and CTA logic: centralized duplicate detection and relink visibility into controller, improved task counters, adjusted redirect notices, and fixed form indexing. * Dang rabbit never stops... Centralized SimpleFin maps logic into `MapsHelper` concern and integrated it into relevant controllers and rake tasks. Optimized queries, reduced redundancy, and improved unlinked counts and manual account checks with batch processing. Adjusted task arguments for clarity. * Persistent rabbit. Optimized SimpleFin maps logic by implementing batch queries for manual account and unlinked count checks, reducing N+1 issues. Improved clarity of rake task argument descriptions and error messages for better usability. * Lost a commit somehow, resolved here. Refactored transaction extra details logic by introducing `build_transaction_extra_details` helper to improve clarity, reusability, and reduce view complexity. Enhanced rake tasks with strict dry-run validation and better error handling. Updated schema to allow nullable `merchant_id` and added conditional unique indexes for recurring transactions. * Refactored sensitive data redaction in `simplefin_unlink` task for recursive handling, optimized SQL sanitization in `simplefin_holdings_backfill`, improved error handling in `transactions_helper`, and streamlined day change calculation logic in `Holding` model. * Lint fix * Removed per PR comments. * Also removing per PR comment. * git commit -m "SimpleFIN polish: preserve #manual-accounts wrapper, unify \"manual\" scope, and correct unlinked counts - Preserve #manual-accounts wrapper: switch non-empty updates to turbo_stream.update and background broadcast_update_to; keep empty-path replace to render <div id=\"manual-accounts\"></div> - Unify definition of manual accounts via Account.visible_manual (visible + legacy-nil + no AccountProvider); reuse in controllers, jobs, and helper - Correct setup/unlinked counts: SimplefinItem::Syncer#finalize_setup_counts and maps now consider AccountProvider links (legacy account AND provider must be absent) Deleted: - app/models/simplefin_item/relink_service.rb - app/controllers/concerns/simplefin_items/relink_helpers.rb - app/javascript/controllers/auto_relink_controller.js - app/views/simplefin_items/_relink_modal.html.erb - app/views/simplefin_items/manual_relink.html.erb - app/views/simplefin_items/relink.html.erb - test/services/simplefin_item/relink_service_test.rb Refs: PR #318 unified link/unlink; PR #267 SimpleFIN; follow-up to fix wrapper ID loss and counting drift." * Extend unlinked account check to include "Investment" type * set SimpleFIN item for `balances`, remove redundant unpacking, and improve holdings task error * SimpleFIN: add `errors` action + modal; do not reintroduce legacy relink actions; removed dead helper * FIX simpleFIN linking * Add delay back, tests benefit from it * Put cache back in * Remove empty `rake` task * Small spelling fixes. --------- Signed-off-by: soky srm <[email protected]> Co-authored-by: Josh Waldrep <[email protected]> Co-authored-by: Juan José Mata <[email protected]> Co-authored-by: sokie <[email protected]> Co-authored-by: Dylan Corrales <[email protected]>
3 participants
User can now unlink accounts as well.
Account will then show under "Other accounts", will keep all transactions and balance information.
If wanted, user can also opt to delete the account then.
Summary by CodeRabbit
New Features
UI
Localization
Tests