-
Notifications
You must be signed in to change notification settings - Fork 97
Feature/simplefin integration #94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
df7a137
9561e73
8497703
30d0b25
18b69a4
35693e5
71d720b
2caba0a
8192481
dbcbf89
332f5b0
9e557df
aea9b06
0c14cca
00cb130
43b3399
4f6b738
1d67188
7fee3fe
e92751d
7685d95
8776faf
2717200
a088a7c
e8b54bc
6681537
77adfc4
020a261
d93235b
d78b172
83994b0
2d82239
b8c2edb
b8b6acb
877ea26
0dfa0b7
8bd2fd0
d2cbea7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| class SimplefinItemsController < ApplicationController | ||
| before_action :set_simplefin_item, only: [ :show, :destroy, :sync, :setup_accounts, :complete_account_setup ] | ||
|
|
||
| def index | ||
| @simplefin_items = Current.family.simplefin_items.active.ordered | ||
| render layout: "settings" | ||
| end | ||
|
|
||
| def show | ||
| end | ||
|
|
||
| def new | ||
| @simplefin_item = Current.family.simplefin_items.build | ||
| end | ||
|
|
||
| def create | ||
| setup_token = simplefin_params[:setup_token] | ||
|
|
||
| return render_error("Please enter a SimpleFin setup token.") if setup_token.blank? | ||
|
|
||
| begin | ||
| @simplefin_item = Current.family.create_simplefin_item!( | ||
| setup_token: setup_token, | ||
| item_name: "SimpleFin Connection" | ||
| ) | ||
|
|
||
| redirect_to simplefin_items_path, notice: "SimpleFin connection added successfully! Your accounts will appear shortly as they sync in the background." | ||
| rescue ArgumentError, URI::InvalidURIError | ||
| render_error("Invalid setup token. Please check that you copied the complete token from SimpleFin Bridge.", setup_token) | ||
| rescue Provider::Simplefin::SimplefinError => e | ||
| error_message = case e.error_type | ||
| when :token_compromised | ||
| "The setup token may be compromised, expired, or already used. Please create a new one." | ||
| else | ||
| "Failed to connect: #{e.message}" | ||
| end | ||
| render_error(error_message, setup_token) | ||
| rescue => e | ||
| Rails.logger.error("SimpleFin connection error: #{e.message}") | ||
| render_error("An unexpected error occurred. Please try again or contact support.", setup_token) | ||
| end | ||
| end | ||
|
|
||
| def destroy | ||
| @simplefin_item.destroy_later | ||
| redirect_to simplefin_items_path, notice: "SimpleFin connection will be removed" | ||
| end | ||
|
|
||
| def sync | ||
| @simplefin_item.sync_later | ||
| redirect_to simplefin_item_path(@simplefin_item), notice: "Sync started" | ||
| end | ||
|
|
||
| def setup_accounts | ||
| @simplefin_accounts = @simplefin_item.simplefin_accounts.includes(:account).where(accounts: { id: nil }) | ||
| @account_type_options = [ | ||
| [ "Checking or Savings Account", "Depository" ], | ||
| [ "Credit Card", "CreditCard" ], | ||
| [ "Investment Account", "Investment" ], | ||
| [ "Loan or Mortgage", "Loan" ], | ||
| [ "Other Asset", "OtherAsset" ], | ||
| [ "Skip - don't add", "Skip" ] | ||
| ] | ||
|
|
||
| # Subtype options for each account type | ||
| @subtype_options = { | ||
| "Depository" => { | ||
| label: "Account Subtype:", | ||
| options: Depository::SUBTYPES.map { |k, v| [ v[:long], k ] } | ||
| }, | ||
| "CreditCard" => { | ||
| label: "", | ||
| options: [], | ||
| message: "Credit cards will be automatically set up as credit card accounts." | ||
| }, | ||
| "Investment" => { | ||
| label: "Investment Type:", | ||
| options: Investment::SUBTYPES.map { |k, v| [ v[:long], k ] } | ||
| }, | ||
| "Loan" => { | ||
| label: "Loan Type:", | ||
| options: Loan::SUBTYPES.map { |k, v| [ v[:long], k ] } | ||
| }, | ||
| "OtherAsset" => { | ||
| label: nil, | ||
| options: [], | ||
| message: "No additional options needed for Other Assets." | ||
| } | ||
| } | ||
| end | ||
|
|
||
| def complete_account_setup | ||
| account_types = params[:account_types] || {} | ||
| account_subtypes = params[:account_subtypes] || {} | ||
|
|
||
| account_types.each do |simplefin_account_id, selected_type| | ||
| # Skip accounts that the user chose not to add | ||
| next if selected_type == "Skip" | ||
|
Comment on lines
+92
to
+98
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix the constant reference error that causes crashes when accessing credit card accounts Based on the PR comments, users are encountering The error is likely not in this exact line but related to how the Skip value is being processed. Let me verify if there's an issue with the account creation logic that might be causing the reported errors. #!/bin/bash
# Check for any references to Skip as a constant in the codebase
rg "Skip[^'\"]" --type ruby
# Check the Account model's create_from_simplefin_account method implementation
ast-grep --pattern 'def create_from_simplefin_account($$$)'
# Look for any Skip-related constant definitions
rg "^\s*(class|module)\s+Skip" --type ruby |
||
|
|
||
| simplefin_account = @simplefin_item.simplefin_accounts.find(simplefin_account_id) | ||
| selected_subtype = account_subtypes[simplefin_account_id] | ||
|
|
||
| # Default subtype for CreditCard since it only has one option | ||
| selected_subtype = "credit_card" if selected_type == "CreditCard" && selected_subtype.blank? | ||
|
|
||
| # Create account with user-selected type and subtype | ||
| account = Account.create_from_simplefin_account( | ||
| simplefin_account, | ||
| selected_type, | ||
| selected_subtype | ||
| ) | ||
| simplefin_account.update!(account: account) | ||
| end | ||
|
|
||
| # Clear pending status and mark as complete | ||
| @simplefin_item.update!(pending_account_setup: false) | ||
|
|
||
| # Schedule account syncs for the newly created accounts | ||
| @simplefin_item.schedule_account_syncs | ||
|
|
||
| redirect_to simplefin_items_path, notice: "SimpleFin accounts have been set up successfully!" | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def set_simplefin_item | ||
| @simplefin_item = Current.family.simplefin_items.find(params[:id]) | ||
| end | ||
|
|
||
| def simplefin_params | ||
| params.require(:simplefin_item).permit(:setup_token) | ||
| end | ||
|
|
||
| def render_error(message, setup_token = nil) | ||
| @simplefin_item = Current.family.simplefin_items.build(setup_token: setup_token) | ||
| @error_message = message | ||
| render :new, status: :unprocessable_entity | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import { Controller } from "@hotwired/stimulus" | ||
|
|
||
| export default class extends Controller { | ||
| static targets = ["subtypeContainer"] | ||
| static values = { accountId: String } | ||
|
|
||
| connect() { | ||
| // Show initial subtype dropdown based on current selection | ||
| this.updateSubtype() | ||
| } | ||
|
|
||
| updateSubtype(event) { | ||
| const selectElement = this.element.querySelector('select[name^="account_types"]') | ||
| const selectedType = selectElement ? selectElement.value : '' | ||
| const container = this.subtypeContainerTarget | ||
| const accountId = this.accountIdValue | ||
|
|
||
| // Hide all subtype selects | ||
| const subtypeSelects = container.querySelectorAll('.subtype-select') | ||
| subtypeSelects.forEach(select => { | ||
| select.style.display = 'none' | ||
| // Clear the name attribute so it doesn't get submitted | ||
| const selectElement = select.querySelector('select') | ||
| if (selectElement) { | ||
| selectElement.removeAttribute('name') | ||
| } | ||
| }) | ||
|
|
||
| // Don't show any subtype select for Skip option | ||
| if (selectedType === 'Skip') { | ||
| return | ||
| } | ||
|
|
||
| // Show the relevant subtype select | ||
| const relevantSubtype = container.querySelector(`[data-type="${selectedType}"]`) | ||
| if (relevantSubtype) { | ||
| relevantSubtype.style.display = 'block' | ||
| // Re-add the name attribute so it gets submitted | ||
| const selectElement = relevantSubtype.querySelector('select') | ||
| if (selectElement) { | ||
| selectElement.setAttribute('name', `account_subtypes[${accountId}]`) | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ class Account < ApplicationRecord | |||||||||||||||
|
|
||||||||||||||||
| belongs_to :family | ||||||||||||||||
| belongs_to :import, optional: true | ||||||||||||||||
| belongs_to :simplefin_account, optional: true | ||||||||||||||||
|
|
||||||||||||||||
| has_many :import_mappings, as: :mappable, dependent: :destroy, class_name: "Import::Mapping" | ||||||||||||||||
| has_many :entries, dependent: :destroy | ||||||||||||||||
|
|
@@ -22,11 +23,12 @@ class Account < ApplicationRecord | |||||||||||||||
| scope :assets, -> { where(classification: "asset") } | ||||||||||||||||
| scope :liabilities, -> { where(classification: "liability") } | ||||||||||||||||
| scope :alphabetically, -> { order(:name) } | ||||||||||||||||
| scope :manual, -> { where(plaid_account_id: nil) } | ||||||||||||||||
| scope :manual, -> { where(plaid_account_id: nil, simplefin_account_id: nil) } | ||||||||||||||||
|
|
||||||||||||||||
| has_one_attached :logo | ||||||||||||||||
|
|
||||||||||||||||
| delegated_type :accountable, types: Accountable::TYPES, dependent: :destroy | ||||||||||||||||
| delegate :subtype, to: :accountable, allow_nil: true | ||||||||||||||||
|
|
||||||||||||||||
| accepts_nested_attributes_for :accountable, update_only: true | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -71,6 +73,53 @@ def create_and_sync(attributes) | |||||||||||||||
| account.sync_later | ||||||||||||||||
| account | ||||||||||||||||
| end | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| def create_from_simplefin_account(simplefin_account, account_type, subtype = nil) | ||||||||||||||||
| # Get the balance from SimpleFin | ||||||||||||||||
| balance = simplefin_account.current_balance || simplefin_account.available_balance || 0 | ||||||||||||||||
|
|
||||||||||||||||
| # SimpleFin returns negative balances for credit cards (liabilities) | ||||||||||||||||
| # But Maybe expects positive balances for liabilities | ||||||||||||||||
| if account_type == "CreditCard" || account_type == "Loan" | ||||||||||||||||
| balance = balance.abs | ||||||||||||||||
| end | ||||||||||||||||
|
Comment on lines
+84
to
+86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainVerify balance conversion logic The comment states SimpleFin returns negative balances for credit cards, but the logic uses 🌐 Web query: 💡 Result: Short answer: SimpleFIN itself does not guarantee that loan account balances will be returned as negative. The SimpleFIN protocol documents the balance and transaction-sign conventions but does not mandate “loan = negative” semantics — institutions/servers can and do return either sign. (simplefin.org) Supporting details and recommendations
Practical approach (recommended)
If you want, tell me:
Citations:
Verify liability account sign normalization The SimpleFIN spec does not guarantee that loan balances will be returned as negative—implementations vary by institution. Right now you’re calling Please confirm your intended sign convention and adjust accordingly:
🤖 Prompt for AI Agents |
||||||||||||||||
|
|
||||||||||||||||
| attributes = { | ||||||||||||||||
| family: simplefin_account.simplefin_item.family, | ||||||||||||||||
| name: simplefin_account.name, | ||||||||||||||||
| balance: balance, | ||||||||||||||||
| currency: simplefin_account.currency, | ||||||||||||||||
| accountable_type: account_type, | ||||||||||||||||
| accountable_attributes: build_simplefin_accountable_attributes(simplefin_account, account_type, subtype), | ||||||||||||||||
| simplefin_account_id: simplefin_account.id | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| create_and_sync(attributes) | ||||||||||||||||
| end | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| private | ||||||||||||||||
|
|
||||||||||||||||
| def build_simplefin_accountable_attributes(simplefin_account, account_type, subtype) | ||||||||||||||||
| attributes = {} | ||||||||||||||||
| attributes[:subtype] = subtype if subtype.present? | ||||||||||||||||
|
|
||||||||||||||||
| # Set account-type-specific attributes from SimpleFin data | ||||||||||||||||
| case account_type | ||||||||||||||||
| when "CreditCard" | ||||||||||||||||
| # For credit cards, available_balance often represents available credit | ||||||||||||||||
| if simplefin_account.available_balance.present? && simplefin_account.available_balance > 0 | ||||||||||||||||
| attributes[:available_credit] = simplefin_account.available_balance | ||||||||||||||||
|
Comment on lines
+112
to
+113
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainPotential bug with balance logic for credit cards Based on the PR comments, there's an issue with credit card accounts showing "unknown attribute 'balance' for CreditCard". The condition checks if when "CreditCard"
# For credit cards, available_balance often represents available credit
- if simplefin_account.available_balance.present? && simplefin_account.available_balance > 0
- attributes[:available_credit] = simplefin_account.available_balance
+ # SimpleFin may return negative values for available credit
+ if simplefin_account.available_balance.present?
+ attributes[:available_credit] = simplefin_account.available_balance.abs
end🏁 Script executed: #!/bin/bash
# Check if CreditCard model has a balance attribute or if it's only on Account
echo "Checking CreditCard model attributes..."
rg "class CreditCard" app/models --type ruby -A 20
# Check schema for credit_cards table
rg "create_table.*credit_cards" db/schema.rb -A 15
# Check if there's a balance attribute definition
rg "attribute.*balance|attr_.*balance" app/models/credit_card.rbLength of output: 1838 Update credit card available_credit logic to handle negative balances For CreditCard accounts, SimpleFin may return a negative • File: when "CreditCard"
# SimpleFin may return negative values for available credit
- if simplefin_account.available_balance.present? && simplefin_account.available_balance > 0
- attributes[:available_credit] = simplefin_account.available_balance
+ if simplefin_account.available_balance.present?
+ attributes[:available_credit] = simplefin_account.available_balance.abs
end📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
| end | ||||||||||||||||
| when "Loan" | ||||||||||||||||
| # For loans, we might get additional data from the raw_payload | ||||||||||||||||
| # This is where loan-specific information could be extracted if available | ||||||||||||||||
| # Currently we don't have specific loan fields from SimpleFin protocol | ||||||||||||||||
| end | ||||||||||||||||
|
|
||||||||||||||||
| attributes | ||||||||||||||||
| end | ||||||||||||||||
| end | ||||||||||||||||
|
|
||||||||||||||||
| def institution_domain | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,11 +3,12 @@ module Account::Linkable | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| included do | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| belongs_to :plaid_account, optional: true | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| belongs_to :simplefin_account, optional: true | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Association looks good; add exclusivity guard to prevent double-linking (Plaid + SimpleFin). Right now an account could potentially have both plaid_account_id and simplefin_account_id set. Enforce mutual exclusivity to avoid ambiguous data sources. Apply this diff: included do
belongs_to :plaid_account, optional: true
belongs_to :simplefin_account, optional: true
+ validate :provider_link_exclusivity
end
+
+ private
+ def provider_link_exclusivity
+ if plaid_account_id.present? && simplefin_account_id.present?
+ errors.add(:base, "Account cannot be linked to multiple providers (Plaid and SimpleFin)")
+ end
+ end📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| # A "linked" account gets transaction and balance data from a third party like Plaid | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # A "linked" account gets transaction and balance data from a third party like Plaid or SimpleFin | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| def linked? | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| plaid_account_id.present? | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| plaid_account_id.present? || simplefin_account_id.present? | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| # An "offline" or "unlinked" account is one where the user tracks values and | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| module Family::SimplefinConnectable | ||
| extend ActiveSupport::Concern | ||
|
|
||
| included do | ||
| has_many :simplefin_items, dependent: :destroy | ||
| end | ||
|
|
||
| def can_connect_simplefin? | ||
| true # SimpleFin doesn't have regional restrictions like Plaid | ||
| end | ||
|
|
||
| def create_simplefin_item!(setup_token:, item_name: nil) | ||
| simplefin_provider = Provider::Simplefin.new | ||
| access_url = simplefin_provider.claim_access_url(setup_token) | ||
|
|
||
| simplefin_item = simplefin_items.create!( | ||
| name: item_name || "SimpleFin Connection", | ||
| access_url: access_url | ||
| ) | ||
|
|
||
| simplefin_item.sync_later | ||
|
|
||
| simplefin_item | ||
| end | ||
Himmelschmidt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider pinning HTTParty to a specific version range.
Adding
httpartywithout version constraints could lead to unexpected behavior if breaking changes are introduced in future releases. For production stability, it's recommended to specify a version constraint.Apply this diff to pin HTTParty to a stable version range:
📝 Committable suggestion
🤖 Prompt for AI Agents