From 4fb0a3856ee22fc41bc05efa7498e8c40856cd0c Mon Sep 17 00:00:00 2001 From: soky srm Date: Tue, 28 Oct 2025 19:32:27 +0100 Subject: [PATCH] Providers factory (#250) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Implement providers factory * Multiple providers sync support - Proper Multi-Provider Syncing: When you click sync on an account with multiple providers (e.g., both Plaid and SimpleFin), all provider items are synced - Better API: The existing account.providers method already returns all providers, and account.provider returns the first one for backward compatibility - Correct Holdings Deletion Logic: Holdings can only be deleted if ALL providers allow it, preventing accidental deletions that would be recreated on next sync TODO: validate this is the way we want to go? We would need to check holdings belong to which account, and then check provider allows deletion. More complex - Database Constraints: The existing validations ensure an account can have at most one provider of each type (one PlaidAccount, one SimplefinAccount, etc.) * Add generic provider_import_adapter * Finish unified import strategy * Update app/models/plaid_account.rb Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: soky srm * Update app/models/provider/factory.rb Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: soky srm * Fix account linked by plaid_id instead of external_id * Parse numerics to BigDecimal Parse numerics to BigDecimal before computing amount; guard nils. Avoid String * String and float drift; also normalize date. * Fix incorrect usage of assert_raises. * Fix linter * Fix processor test. * Update current_balance_manager.rb * Test fixes * Fix plaid linked account test * Add support for holding per account_provider * Fix proper account access Also fix account deletion for simpefin too * FIX match tests for consistency * Some more factory updates * Fix account schema for multipe providers Can do: - Account #1 → PlaidAccount + SimplefinAccount (multiple different providers) - Account #2 → PlaidAccount only - Account #3 → SimplefinAccount only Cannot do: - Account #1 → PlaidAccount + PlaidAccount (duplicate provider type) - PlaidAccount #123 → Account #1 + Account #2 (provider linked to multiple accounts) * Fix account setup - An account CAN have multiple providers (the schema shows account_providers with unique index on [account_id, provider_type]) - Each provider should maintain its own separate entries - We should NOT update one provider's entry when another provider syncs * Fix linter and guard migration * FIX linter issues. * Fixes - Remove duplicated index - Pass account_provider_id - Guard holdings call to avoid NoMethodError * Update schema and provider import fix * Plaid doesn't allow holdings deletion * Use ClimateControl for proper env setup * No need for this in .git --------- Signed-off-by: soky srm Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Juan José Mata --- .gitignore | 1 + app/controllers/accounts_controller.rb | 12 +- app/controllers/holdings_controller.rb | 6 +- app/helpers/accounts_helper.rb | 9 +- app/models/account.rb | 16 +- app/models/account/current_balance_manager.rb | 6 +- app/models/account/linkable.rb | 44 +- app/models/account/provider_import_adapter.rb | 255 ++++++++ app/models/account_provider.rb | 18 + app/models/assistant/function/get_accounts.rb | 5 +- app/models/entry.rb | 3 +- app/models/holding.rb | 1 + app/models/plaid_account.rb | 11 +- .../investments/holdings_processor.rb | 84 ++- .../investments/transactions_processor.rb | 52 +- .../liabilities/credit_processor.rb | 15 +- .../liabilities/mortgage_processor.rb | 2 +- .../liabilities/student_loan_processor.rb | 2 +- app/models/plaid_account/processor.rb | 26 +- .../plaid_account/transactions/processor.rb | 2 +- app/models/plaid_entry/processor.rb | 80 +-- app/models/provider/base.rb | 69 +++ app/models/provider/factory.rb | 66 ++ app/models/provider/institution_metadata.rb | 40 ++ app/models/provider/plaid_adapter.rb | 48 ++ app/models/provider/simplefin_adapter.rb | 60 ++ app/models/provider/syncable.rb | 35 ++ app/models/simplefin_account.rb | 12 +- .../investments/holdings_processor.rb | 47 +- .../investments/transactions_processor.rb | 35 +- .../liabilities/credit_processor.rb | 18 +- .../liabilities/loan_processor.rb | 4 +- app/models/simplefin_account/processor.rb | 10 +- .../transactions/processor.rb | 2 +- app/models/simplefin_entry/processor.rb | 44 +- app/views/accounts/_logo.html.erb | 2 +- app/views/accounts/show/_activity.html.erb | 2 +- app/views/holdings/show.html.erb | 2 +- app/views/simplefin_items/show.html.erb | 4 +- ...20251027085448_create_account_providers.rb | 15 + ...27085614_migrate_account_providers_data.rb | 36 ++ ...2_add_external_id_and_source_to_entries.rb | 9 + ...provider_merchant_id_index_to_merchants.rb | 9 + ...141735_add_account_provider_to_holdings.rb | 5 + ...028104241_fix_account_providers_indexes.rb | 18 + ...kfill_entries_external_id_from_plaid_id.rb | 26 + ...104916_update_entries_external_id_index.rb | 25 + ...8174016_remove_duplicate_holdings_index.rb | 5 + db/schema.rb | 23 +- test/controllers/accounts_controller_test.rb | 25 + .../simplefin_items_controller_test.rb | 6 +- .../account/current_balance_manager_test.rb | 7 + test/models/account/linkable_test.rb | 69 +++ .../account/provider_import_adapter_test.rb | 569 ++++++++++++++++++ test/models/account_provider_test.rb | 132 ++++ .../investments/holdings_processor_test.rb | 173 +++++- .../transactions_processor_test.rb | 9 +- .../liabilities/credit_processor_test.rb | 10 +- .../liabilities/mortgage_processor_test.rb | 6 +- .../student_loan_processor_test.rb | 8 +- test/models/plaid_account/processor_test.rb | 31 +- .../transactions/processor_test.rb | 2 +- test/models/plaid_entry/processor_test.rb | 5 +- test/models/provider/plaid_adapter_test.rb | 41 ++ test/models/provider/registry_test.rb | 27 +- .../models/provider/simplefin_adapter_test.rb | 72 +++ .../provider_adapter_test_interface.rb | 140 +++++ 67 files changed, 2338 insertions(+), 315 deletions(-) create mode 100644 app/models/account/provider_import_adapter.rb create mode 100644 app/models/account_provider.rb create mode 100644 app/models/provider/base.rb create mode 100644 app/models/provider/factory.rb create mode 100644 app/models/provider/institution_metadata.rb create mode 100644 app/models/provider/plaid_adapter.rb create mode 100644 app/models/provider/simplefin_adapter.rb create mode 100644 app/models/provider/syncable.rb create mode 100644 db/migrate/20251027085448_create_account_providers.rb create mode 100644 db/migrate/20251027085614_migrate_account_providers_data.rb create mode 100644 db/migrate/20251027110502_add_external_id_and_source_to_entries.rb create mode 100644 db/migrate/20251027110609_add_provider_merchant_id_index_to_merchants.rb create mode 100644 db/migrate/20251027141735_add_account_provider_to_holdings.rb create mode 100644 db/migrate/20251028104241_fix_account_providers_indexes.rb create mode 100644 db/migrate/20251028104851_backfill_entries_external_id_from_plaid_id.rb create mode 100644 db/migrate/20251028104916_update_entries_external_id_index.rb create mode 100644 db/migrate/20251028174016_remove_duplicate_holdings_index.rb create mode 100644 test/models/account/linkable_test.rb create mode 100644 test/models/account/provider_import_adapter_test.rb create mode 100644 test/models/account_provider_test.rb create mode 100644 test/models/provider/plaid_adapter_test.rb create mode 100644 test/models/provider/simplefin_adapter_test.rb create mode 100644 test/support/provider_adapter_test_interface.rb diff --git a/.gitignore b/.gitignore index 4fbcee9d6..3bed5cc48 100644 --- a/.gitignore +++ b/.gitignore @@ -30,6 +30,7 @@ /tmp/storage/* !/tmp/storage/ !/tmp/storage/.keep +/db/development.sqlite3 /public/assets diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index e394d6ce9..dd4b08cdd 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -28,7 +28,17 @@ class AccountsController < ApplicationController def sync unless @account.syncing? - @account.sync_later + if @account.linked? + # Sync all provider items for this account + # Each provider item will trigger an account sync when complete + @account.account_providers.each do |account_provider| + item = account_provider.adapter&.item + item&.sync_later if item && !item.syncing? + end + else + # Manual accounts just need balance materialization + @account.sync_later + end end redirect_to account_path(@account) diff --git a/app/controllers/holdings_controller.rb b/app/controllers/holdings_controller.rb index db9d59b44..96ce2cd49 100644 --- a/app/controllers/holdings_controller.rb +++ b/app/controllers/holdings_controller.rb @@ -9,11 +9,11 @@ class HoldingsController < ApplicationController end def destroy - if @holding.account.plaid_account_id.present? - flash[:alert] = "You cannot delete this holding" - else + if @holding.account.can_delete_holdings? @holding.destroy_holding_and_entries! flash[:notice] = t(".success") + else + flash[:alert] = "You cannot delete this holding" end respond_to do |format| diff --git a/app/helpers/accounts_helper.rb b/app/helpers/accounts_helper.rb index de809abe4..99ef891ce 100644 --- a/app/helpers/accounts_helper.rb +++ b/app/helpers/accounts_helper.rb @@ -5,12 +5,7 @@ module AccountsHelper end def sync_path_for(account) - if account.plaid_account_id.present? - sync_plaid_item_path(account.plaid_account.plaid_item) - elsif account.simplefin_account_id.present? - sync_simplefin_item_path(account.simplefin_account.simplefin_item) - else - sync_account_path(account) - end + # Always use the account sync path, which handles syncing all providers + sync_account_path(account) end end diff --git a/app/models/account.rb b/app/models/account.rb index 4c2e7eb06..b55aafd37 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -5,7 +5,6 @@ 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 @@ -23,7 +22,7 @@ class Account < ApplicationRecord scope :assets, -> { where(classification: "asset") } scope :liabilities, -> { where(classification: "liability") } scope :alphabetically, -> { order(:name) } - scope :manual, -> { where(plaid_account_id: nil, simplefin_account_id: nil) } + scope :manual, -> { left_joins(:account_providers).where(account_providers: { id: nil }) } has_one_attached :logo @@ -141,18 +140,7 @@ class Account < ApplicationRecord end def institution_domain - url_string = plaid_account&.plaid_item&.institution_url - return nil unless url_string.present? - - begin - uri = URI.parse(url_string) - # Use safe navigation on .host before calling gsub - uri.host&.gsub(/^www\./, "") - rescue URI::InvalidURIError - # Log a warning if the URL is invalid and return nil - Rails.logger.warn("Invalid institution URL encountered for account #{id}: #{url_string}") - nil - end + provider&.institution_domain end def destroy_later diff --git a/app/models/account/current_balance_manager.rb b/app/models/account/current_balance_manager.rb index 2b1ba6309..f1c5928a5 100644 --- a/app/models/account/current_balance_manager.rb +++ b/app/models/account/current_balance_manager.rb @@ -107,13 +107,17 @@ class Account::CurrentBalanceManager end def create_current_anchor(balance) - account.entries.create!( + entry = account.entries.create!( date: Date.current, name: Valuation.build_current_anchor_name(account.accountable_type), amount: balance, currency: account.currency, entryable: Valuation.new(kind: "current_anchor") ) + + # Reload associations and clear memoized value so it gets the new anchor + account.valuations.reload + @current_anchor_valuation = nil end def update_current_anchor(balance) diff --git a/app/models/account/linkable.rb b/app/models/account/linkable.rb index 2a57e71c2..328cf5f10 100644 --- a/app/models/account/linkable.rb +++ b/app/models/account/linkable.rb @@ -2,13 +2,17 @@ module Account::Linkable extend ActiveSupport::Concern included do + # New generic provider association + has_many :account_providers, dependent: :destroy + + # Legacy provider associations - kept for backward compatibility during migration belongs_to :plaid_account, optional: true belongs_to :simplefin_account, optional: true end # A "linked" account gets transaction and balance data from a third party like Plaid or SimpleFin def linked? - plaid_account_id.present? || simplefin_account_id.present? + account_providers.any? end # An "offline" or "unlinked" account is one where the user tracks values and @@ -17,4 +21,42 @@ module Account::Linkable !linked? end alias_method :manual?, :unlinked? + + # Returns the primary provider adapter for this account + # If multiple providers exist, returns the first one + def provider + return nil unless linked? + + @provider ||= account_providers.first&.adapter + end + + # Returns all provider adapters for this account + def providers + @providers ||= account_providers.map(&:adapter).compact + end + + # Returns the provider adapter for a specific provider type + def provider_for(provider_type) + account_provider = account_providers.find_by(provider_type: provider_type) + account_provider&.adapter + end + + # Convenience method to get the provider name + def provider_name + provider&.provider_name + end + + # Check if account is linked to a specific provider + def linked_to?(provider_type) + account_providers.exists?(provider_type: provider_type) + end + + # Check if holdings can be deleted + # If account has multiple providers, returns true only if ALL providers allow deletion + # This prevents deleting holdings that would be recreated on next sync + def can_delete_holdings? + return true if unlinked? + + providers.all?(&:can_delete_holdings?) + end end diff --git a/app/models/account/provider_import_adapter.rb b/app/models/account/provider_import_adapter.rb new file mode 100644 index 000000000..ba3deaece --- /dev/null +++ b/app/models/account/provider_import_adapter.rb @@ -0,0 +1,255 @@ +class Account::ProviderImportAdapter + attr_reader :account + + def initialize(account) + @account = account + end + + # Imports a transaction from a provider + # + # @param external_id [String] Unique identifier from the provider (e.g., "plaid_12345", "simplefin_abc") + # @param amount [BigDecimal, Numeric] Transaction amount + # @param currency [String] Currency code (e.g., "USD") + # @param date [Date, String] Transaction date + # @param name [String] Transaction name/description + # @param source [String] Provider name (e.g., "plaid", "simplefin") + # @param category_id [Integer, nil] Optional category ID + # @param merchant [Merchant, nil] Optional merchant object + # @return [Entry] The created or updated entry + def import_transaction(external_id:, amount:, currency:, date:, name:, source:, category_id: nil, merchant: nil) + raise ArgumentError, "external_id is required" if external_id.blank? + raise ArgumentError, "source is required" if source.blank? + + Account.transaction do + # Find or initialize by both external_id AND source + # This allows multiple providers to sync same account with separate entries + entry = account.entries.find_or_initialize_by(external_id: external_id, source: source) do |e| + e.entryable = Transaction.new + end + + # Validate entryable type matches to prevent external_id collisions + if entry.persisted? && !entry.entryable.is_a?(Transaction) + raise ArgumentError, "Entry with external_id '#{external_id}' already exists with different entryable type: #{entry.entryable_type}" + end + + entry.assign_attributes( + amount: amount, + currency: currency, + date: date + ) + + # Use enrichment pattern to respect user overrides + entry.enrich_attribute(:name, name, source: source) + + # Enrich transaction-specific attributes + if category_id + entry.transaction.enrich_attribute(:category_id, category_id, source: source) + end + + if merchant + entry.transaction.enrich_attribute(:merchant_id, merchant.id, source: source) + end + + entry.save! + entry + end + end + + # Finds or creates a merchant from provider data + # + # @param provider_merchant_id [String] Provider's merchant ID + # @param name [String] Merchant name + # @param source [String] Provider name (e.g., "plaid", "simplefin") + # @param website_url [String, nil] Optional merchant website + # @param logo_url [String, nil] Optional merchant logo URL + # @return [ProviderMerchant, nil] The merchant object or nil if data is insufficient + def find_or_create_merchant(provider_merchant_id:, name:, source:, website_url: nil, logo_url: nil) + return nil unless provider_merchant_id.present? && name.present? + + ProviderMerchant.find_or_create_by!( + provider_merchant_id: provider_merchant_id, + source: source + ) do |m| + m.name = name + m.website_url = website_url + m.logo_url = logo_url + end + end + + # Updates account balance from provider data + # + # @param balance [BigDecimal, Numeric] Total balance + # @param cash_balance [BigDecimal, Numeric] Cash balance (for investment accounts) + # @param source [String] Provider name (for logging/debugging) + def update_balance(balance:, cash_balance: nil, source: nil) + account.update!( + balance: balance, + cash_balance: cash_balance || balance + ) + end + + # Imports or updates a holding (investment position) from a provider + # + # @param security [Security] The security object + # @param quantity [BigDecimal, Numeric] Number of shares/units + # @param amount [BigDecimal, Numeric] Total value in account currency + # @param currency [String] Currency code + # @param date [Date, String] Holding date + # @param price [BigDecimal, Numeric, nil] Price per share (optional) + # @param cost_basis [BigDecimal, Numeric, nil] Cost basis (optional) + # @param external_id [String, nil] Provider's unique ID (optional, for deduplication) + # @param source [String] Provider name + # @param account_provider_id [String, nil] The AccountProvider ID that owns this holding (optional) + # @param delete_future_holdings [Boolean] Whether to delete holdings after this date (default: false) + # @return [Holding] The created or updated holding + def import_holding(security:, quantity:, amount:, currency:, date:, price: nil, cost_basis: nil, external_id: nil, source:, account_provider_id: nil, delete_future_holdings: false) + raise ArgumentError, "security is required" if security.nil? + raise ArgumentError, "source is required" if source.blank? + + Account.transaction do + # Two strategies for finding/creating holdings: + # 1. By external_id (SimpleFin approach) - tracks each holding uniquely + # 2. By security+date+currency (Plaid approach) - overwrites holdings for same security/date + holding = if external_id.present? + account.holdings.find_or_initialize_by(external_id: external_id) do |h| + h.security = security + h.date = date + h.currency = currency + end + else + account.holdings.find_or_initialize_by( + security: security, + date: date, + currency: currency + ) + end + + holding.assign_attributes( + security: security, + date: date, + currency: currency, + qty: quantity, + price: price, + amount: amount, + cost_basis: cost_basis, + account_provider_id: account_provider_id + ) + + holding.save! + + # Optionally delete future holdings for this security (Plaid behavior) + # Only delete if ALL providers allow deletion (cross-provider check) + if delete_future_holdings + unless account.can_delete_holdings? + Rails.logger.warn( + "Skipping future holdings deletion for account #{account.id} " \ + "because not all providers allow deletion" + ) + return holding + end + + # Build base query for future holdings + future_holdings_query = account.holdings + .where(security: security) + .where("date > ?", date) + + # If account_provider_id is provided, only delete holdings from this provider + # This prevents deleting positions imported by other providers + if account_provider_id.present? + future_holdings_query = future_holdings_query.where(account_provider_id: account_provider_id) + end + + future_holdings_query.destroy_all + end + + holding + end + end + + # Imports a trade (investment transaction) from a provider + # + # @param security [Security] The security object + # @param quantity [BigDecimal, Numeric] Number of shares (negative for sells, positive for buys) + # @param price [BigDecimal, Numeric] Price per share + # @param amount [BigDecimal, Numeric] Total trade value + # @param currency [String] Currency code + # @param date [Date, String] Trade date + # @param name [String, nil] Optional custom name for the trade + # @param external_id [String, nil] Provider's unique ID (optional, for deduplication) + # @param source [String] Provider name + # @return [Entry] The created entry with trade + def import_trade(security:, quantity:, price:, amount:, currency:, date:, name: nil, external_id: nil, source:) + raise ArgumentError, "security is required" if security.nil? + raise ArgumentError, "source is required" if source.blank? + + Account.transaction do + # Generate name if not provided + trade_name = if name.present? + name + else + trade_type = quantity.negative? ? "sell" : "buy" + Trade.build_name(trade_type, quantity, security.ticker) + end + + # Use find_or_initialize_by with external_id if provided, otherwise create new + entry = if external_id.present? + # Find or initialize by both external_id AND source + # This allows multiple providers to sync same account with separate entries + account.entries.find_or_initialize_by(external_id: external_id, source: source) do |e| + e.entryable = Trade.new + end + else + account.entries.new( + entryable: Trade.new, + source: source + ) + end + + # Validate entryable type matches to prevent external_id collisions + if entry.persisted? && !entry.entryable.is_a?(Trade) + raise ArgumentError, "Entry with external_id '#{external_id}' already exists with different entryable type: #{entry.entryable_type}" + end + + # Always update Trade attributes (works for both new and existing records) + entry.entryable.assign_attributes( + security: security, + qty: quantity, + price: price, + currency: currency + ) + + entry.assign_attributes( + date: date, + amount: amount, + currency: currency, + name: trade_name + ) + + entry.save! + entry + end + end + + # Updates accountable-specific attributes (e.g., credit card details, loan details) + # + # @param attributes [Hash] Hash of attributes to update on the accountable + # @param source [String] Provider name (for logging/debugging) + # @return [Boolean] Whether the update was successful + def update_accountable_attributes(attributes:, source:) + return false unless account.accountable.present? + return false if attributes.blank? + + # Filter out nil values and only update attributes that exist on the accountable + valid_attributes = attributes.compact.select do |key, _| + account.accountable.respond_to?("#{key}=") + end + + return false if valid_attributes.empty? + + account.accountable.update!(valid_attributes) + true + rescue => e + Rails.logger.error("Failed to update #{account.accountable_type} attributes from #{source}: #{e.message}") + false + end +end diff --git a/app/models/account_provider.rb b/app/models/account_provider.rb new file mode 100644 index 000000000..bfb39f508 --- /dev/null +++ b/app/models/account_provider.rb @@ -0,0 +1,18 @@ +class AccountProvider < ApplicationRecord + belongs_to :account + belongs_to :provider, polymorphic: true + + validates :account_id, uniqueness: { scope: :provider_type } + validates :provider_id, uniqueness: { scope: :provider_type } + + # Returns the provider adapter for this connection + def adapter + Provider::Factory.create_adapter(provider, account: account) + end + + # Convenience method to get provider name + # Delegates to the adapter for consistency, falls back to underscored provider_type + def provider_name + adapter&.provider_name || provider_type.underscore + end +end diff --git a/app/models/assistant/function/get_accounts.rb b/app/models/assistant/function/get_accounts.rb index f8c6a6db4..777b81493 100644 --- a/app/models/assistant/function/get_accounts.rb +++ b/app/models/assistant/function/get_accounts.rb @@ -12,7 +12,7 @@ class Assistant::Function::GetAccounts < Assistant::Function def call(params = {}) { as_of_date: Date.current, - accounts: family.accounts.includes(:balances).map do |account| + accounts: family.accounts.includes(:balances, :account_providers).map do |account| { name: account.name, balance: account.balance, @@ -21,7 +21,8 @@ class Assistant::Function::GetAccounts < Assistant::Function classification: account.classification, type: account.accountable_type, start_date: account.start_date, - is_plaid_linked: account.plaid_account_id.present?, + is_linked: account.linked?, + provider: account.provider_name, status: account.status, historical_balances: historical_balances(account) } diff --git a/app/models/entry.rb b/app/models/entry.rb index 332cbee91..3a6672b7d 100644 --- a/app/models/entry.rb +++ b/app/models/entry.rb @@ -13,6 +13,7 @@ class Entry < ApplicationRecord validates :date, :name, :amount, :currency, presence: true validates :date, uniqueness: { scope: [ :account_id, :entryable_type ] }, if: -> { valuation? } validates :date, comparison: { greater_than: -> { min_supported_date } } + validates :external_id, uniqueness: { scope: [ :account_id, :source ] }, if: -> { external_id.present? && source.present? } scope :visible, -> { joins(:account).where(accounts: { status: [ "draft", "active" ] }) @@ -57,7 +58,7 @@ class Entry < ApplicationRecord end def linked? - plaid_id.present? + external_id.present? end class << self diff --git a/app/models/holding.rb b/app/models/holding.rb index 90bf4f49c..ae664a83a 100644 --- a/app/models/holding.rb +++ b/app/models/holding.rb @@ -5,6 +5,7 @@ class Holding < ApplicationRecord belongs_to :account belongs_to :security + belongs_to :account_provider, optional: true validates :qty, :currency, :date, :price, :amount, presence: true validates :qty, :price, :amount, numericality: { greater_than_or_equal_to: 0 } diff --git a/app/models/plaid_account.rb b/app/models/plaid_account.rb index 949167ce5..4217a7926 100644 --- a/app/models/plaid_account.rb +++ b/app/models/plaid_account.rb @@ -1,11 +1,20 @@ class PlaidAccount < ApplicationRecord belongs_to :plaid_item - has_one :account, dependent: :destroy + # Legacy association via foreign key (will be removed after migration) + has_one :account, dependent: :nullify, foreign_key: :plaid_account_id + # New association through account_providers + has_one :account_provider, as: :provider, dependent: :destroy + has_one :linked_account, through: :account_provider, source: :account validates :name, :plaid_type, :currency, presence: true validate :has_balance + # Helper to get account using new system first, falling back to legacy + def current_account + linked_account || account + end + def upsert_plaid_snapshot!(account_snapshot) assign_attributes( current_balance: account_snapshot.balances.current, diff --git a/app/models/plaid_account/investments/holdings_processor.rb b/app/models/plaid_account/investments/holdings_processor.rb index 8dac6baee..493b8a9d1 100644 --- a/app/models/plaid_account/investments/holdings_processor.rb +++ b/app/models/plaid_account/investments/holdings_processor.rb @@ -11,40 +11,82 @@ class PlaidAccount::Investments::HoldingsProcessor next unless resolved_security_result.security.present? security = resolved_security_result.security - holding_date = plaid_holding["institution_price_as_of"] || Date.current - holding = account.holdings.find_or_initialize_by( + # Parse quantity and price into BigDecimal for proper arithmetic + quantity_bd = parse_decimal(plaid_holding["quantity"]) + price_bd = parse_decimal(plaid_holding["institution_price"]) + + # Skip if essential values are missing + next if quantity_bd.nil? || price_bd.nil? + + # Compute amount using BigDecimal arithmetic to avoid floating point drift + amount_bd = quantity_bd * price_bd + + # Normalize date - handle string, Date, or nil + holding_date = parse_date(plaid_holding["institution_price_as_of"]) || Date.current + + import_adapter.import_holding( security: security, + quantity: quantity_bd, + amount: amount_bd, + currency: plaid_holding["iso_currency_code"] || account.currency, date: holding_date, - currency: plaid_holding["iso_currency_code"] + price: price_bd, + account_provider_id: plaid_account.account_provider&.id, + source: "plaid", + delete_future_holdings: false # Plaid doesn't allow holdings deletion ) - - holding.assign_attributes( - qty: plaid_holding["quantity"], - price: plaid_holding["institution_price"], - amount: plaid_holding["quantity"] * plaid_holding["institution_price"] - ) - - ActiveRecord::Base.transaction do - holding.save! - - # Delete all holdings for this security after the institution price date - account.holdings - .where(security: security) - .where("date > ?", holding_date) - .destroy_all - end end end private attr_reader :plaid_account, :security_resolver + def import_adapter + @import_adapter ||= Account::ProviderImportAdapter.new(account) + end + def account - plaid_account.account + plaid_account.current_account end def holdings - plaid_account.raw_investments_payload["holdings"] || [] + plaid_account.raw_investments_payload&.[]("holdings") || [] + end + + def parse_decimal(value) + return nil if value.nil? + + case value + when BigDecimal + value + when String + BigDecimal(value) + when Numeric + BigDecimal(value.to_s) + else + nil + end + rescue ArgumentError => e + Rails.logger.error("Failed to parse Plaid holding decimal value: #{value.inspect} - #{e.message}") + nil + end + + def parse_date(date_value) + return nil if date_value.nil? + + case date_value + when Date + date_value + when String + Date.parse(date_value) + when Time, DateTime + date_value.to_date + else + nil + end + rescue ArgumentError, TypeError => e + Rails.logger.error("Failed to parse Plaid holding date: #{date_value.inspect} - #{e.message}") + nil end end diff --git a/app/models/plaid_account/investments/transactions_processor.rb b/app/models/plaid_account/investments/transactions_processor.rb index df4945047..922a3f2a6 100644 --- a/app/models/plaid_account/investments/transactions_processor.rb +++ b/app/models/plaid_account/investments/transactions_processor.rb @@ -19,8 +19,12 @@ class PlaidAccount::Investments::TransactionsProcessor private attr_reader :plaid_account, :security_resolver + def import_adapter + @import_adapter ||= Account::ProviderImportAdapter.new(account) + end + def account - plaid_account.account + plaid_account.current_account end def cash_transaction?(transaction) @@ -38,50 +42,34 @@ class PlaidAccount::Investments::TransactionsProcessor return # We can't process a non-cash transaction without a security end - entry = account.entries.find_or_initialize_by(plaid_id: transaction["investment_transaction_id"]) do |e| - e.entryable = Trade.new - end + external_id = transaction["investment_transaction_id"] + return if external_id.blank? - entry.assign_attributes( + import_adapter.import_trade( + external_id: external_id, + security: resolved_security_result.security, + quantity: derived_qty(transaction), + price: transaction["price"], amount: derived_qty(transaction) * transaction["price"], currency: transaction["iso_currency_code"], - date: transaction["date"] - ) - - entry.trade.assign_attributes( - security: resolved_security_result.security, - qty: derived_qty(transaction), - price: transaction["price"], - currency: transaction["iso_currency_code"] - ) - - entry.enrich_attribute( - :name, - transaction["name"], + date: transaction["date"], + name: transaction["name"], source: "plaid" ) - - entry.save! end def find_or_create_cash_entry(transaction) - entry = account.entries.find_or_initialize_by(plaid_id: transaction["investment_transaction_id"]) do |e| - e.entryable = Transaction.new - end + external_id = transaction["investment_transaction_id"] + return if external_id.blank? - entry.assign_attributes( + import_adapter.import_transaction( + external_id: external_id, amount: transaction["amount"], currency: transaction["iso_currency_code"], - date: transaction["date"] - ) - - entry.enrich_attribute( - :name, - transaction["name"], + date: transaction["date"], + name: transaction["name"], source: "plaid" ) - - entry.save! end def transactions diff --git a/app/models/plaid_account/liabilities/credit_processor.rb b/app/models/plaid_account/liabilities/credit_processor.rb index cc4872951..7a5b216db 100644 --- a/app/models/plaid_account/liabilities/credit_processor.rb +++ b/app/models/plaid_account/liabilities/credit_processor.rb @@ -6,17 +6,24 @@ class PlaidAccount::Liabilities::CreditProcessor def process return unless credit_data.present? - account.credit_card.update!( - minimum_payment: credit_data.dig("minimum_payment_amount"), - apr: credit_data.dig("aprs", 0, "apr_percentage") + import_adapter.update_accountable_attributes( + attributes: { + minimum_payment: credit_data.dig("minimum_payment_amount"), + apr: credit_data.dig("aprs", 0, "apr_percentage") + }, + source: "plaid" ) end private attr_reader :plaid_account + def import_adapter + @import_adapter ||= Account::ProviderImportAdapter.new(account) + end + def account - plaid_account.account + plaid_account.current_account end def credit_data diff --git a/app/models/plaid_account/liabilities/mortgage_processor.rb b/app/models/plaid_account/liabilities/mortgage_processor.rb index d4610362d..d5744c9a7 100644 --- a/app/models/plaid_account/liabilities/mortgage_processor.rb +++ b/app/models/plaid_account/liabilities/mortgage_processor.rb @@ -16,7 +16,7 @@ class PlaidAccount::Liabilities::MortgageProcessor attr_reader :plaid_account def account - plaid_account.account + plaid_account.current_account end def mortgage_data diff --git a/app/models/plaid_account/liabilities/student_loan_processor.rb b/app/models/plaid_account/liabilities/student_loan_processor.rb index c3c3b4f2e..61d6f484c 100644 --- a/app/models/plaid_account/liabilities/student_loan_processor.rb +++ b/app/models/plaid_account/liabilities/student_loan_processor.rb @@ -18,7 +18,7 @@ class PlaidAccount::Liabilities::StudentLoanProcessor attr_reader :plaid_account def account - plaid_account.account + plaid_account.current_account end def term_months diff --git a/app/models/plaid_account/processor.rb b/app/models/plaid_account/processor.rb index fa898b3b3..4faead9d9 100644 --- a/app/models/plaid_account/processor.rb +++ b/app/models/plaid_account/processor.rb @@ -30,9 +30,20 @@ class PlaidAccount::Processor def process_account! PlaidAccount.transaction do - account = family.accounts.find_or_initialize_by( - plaid_account_id: plaid_account.id - ) + # Find existing account through account_provider or legacy plaid_account_id + account_provider = AccountProvider.find_by(provider: plaid_account) + account = if account_provider + account_provider.account + else + # Legacy fallback: find by plaid_account_id if it still exists + family.accounts.find_by(plaid_account_id: plaid_account.id) + end + + # Initialize new account if not found + if account.nil? + account = family.accounts.new + account.accountable = map_accountable(plaid_account.plaid_type) + end # Create or assign the accountable if needed if account.accountable.nil? @@ -65,6 +76,15 @@ class PlaidAccount::Processor account.save! + # Create account provider link if it doesn't exist + unless account_provider + AccountProvider.find_or_create_by!( + account: account, + provider: plaid_account, + provider_type: "PlaidAccount" + ) + end + # Create or update the current balance anchor valuation for event-sourced ledger # Note: This is a partial implementation. In the future, we'll introduce HoldingValuation # to properly track the holdings vs. cash breakdown, but for now we're only tracking diff --git a/app/models/plaid_account/transactions/processor.rb b/app/models/plaid_account/transactions/processor.rb index 8aa07162a..90e55d2d9 100644 --- a/app/models/plaid_account/transactions/processor.rb +++ b/app/models/plaid_account/transactions/processor.rb @@ -39,7 +39,7 @@ class PlaidAccount::Transactions::Processor end def account - plaid_account.account + plaid_account.current_account end def remove_plaid_transaction(raw_transaction) diff --git a/app/models/plaid_entry/processor.rb b/app/models/plaid_entry/processor.rb index 182e382d8..13d35a93e 100644 --- a/app/models/plaid_entry/processor.rb +++ b/app/models/plaid_entry/processor.rb @@ -7,53 +7,30 @@ class PlaidEntry::Processor end def process - PlaidAccount.transaction do - entry = account.entries.find_or_initialize_by(plaid_id: plaid_id) do |e| - e.entryable = Transaction.new - end - - entry.assign_attributes( - amount: amount, - currency: currency, - date: date - ) - - entry.enrich_attribute( - :name, - name, - source: "plaid" - ) - - if detailed_category - matched_category = category_matcher.match(detailed_category) - - if matched_category - entry.transaction.enrich_attribute( - :category_id, - matched_category.id, - source: "plaid" - ) - end - end - - if merchant - entry.transaction.enrich_attribute( - :merchant_id, - merchant.id, - source: "plaid" - ) - end - end + import_adapter.import_transaction( + external_id: external_id, + amount: amount, + currency: currency, + date: date, + name: name, + source: "plaid", + category_id: matched_category&.id, + merchant: merchant + ) end private attr_reader :plaid_transaction, :plaid_account, :category_matcher - def account - plaid_account.account + def import_adapter + @import_adapter ||= Account::ProviderImportAdapter.new(account) end - def plaid_id + def account + plaid_account.current_account + end + + def external_id plaid_transaction["transaction_id"] end @@ -77,19 +54,18 @@ class PlaidEntry::Processor plaid_transaction.dig("personal_finance_category", "detailed") end + def matched_category + return nil unless detailed_category + @matched_category ||= category_matcher.match(detailed_category) + end + def merchant - merchant_id = plaid_transaction["merchant_entity_id"] - merchant_name = plaid_transaction["merchant_name"] - - return nil unless merchant_id.present? && merchant_name.present? - - ProviderMerchant.find_or_create_by!( + @merchant ||= import_adapter.find_or_create_merchant( + provider_merchant_id: plaid_transaction["merchant_entity_id"], + name: plaid_transaction["merchant_name"], source: "plaid", - name: merchant_name, - ) do |m| - m.provider_merchant_id = merchant_id - m.website_url = plaid_transaction["website"] - m.logo_url = plaid_transaction["logo_url"] - end + website_url: plaid_transaction["website"], + logo_url: plaid_transaction["logo_url"] + ) end end diff --git a/app/models/provider/base.rb b/app/models/provider/base.rb new file mode 100644 index 000000000..22b1263aa --- /dev/null +++ b/app/models/provider/base.rb @@ -0,0 +1,69 @@ +# Base class for all provider adapters +# Provides common interface for working with different third-party data providers +# +# To create a new provider adapter: +# 1. Inherit from Provider::Base +# 2. Implement #provider_name +# 3. Include optional modules (Provider::Syncable, Provider::InstitutionMetadata) +# 4. Register with Provider::Factory in the class body +# +# Example: +# class Provider::AcmeAdapter < Provider::Base +# Provider::Factory.register("AcmeAccount", self) +# include Provider::Syncable +# include Provider::InstitutionMetadata +# +# def provider_name +# "acme" +# end +# end +class Provider::Base + attr_reader :provider_account, :account + + def initialize(provider_account, account: nil) + @provider_account = provider_account + @account = account || provider_account.account + end + + # Provider identification - must be implemented by subclasses + # @return [String] The provider name (e.g., "plaid", "simplefin") + def provider_name + raise NotImplementedError, "#{self.class} must implement #provider_name" + end + + # Returns the provider type (class name) + # @return [String] The provider account class name + def provider_type + provider_account.class.name + end + + # Whether this provider allows deletion of holdings + # Override in subclass if provider supports holdings deletion + # @return [Boolean] True if holdings can be deleted, false otherwise + def can_delete_holdings? + false + end + + # Provider-specific raw data payload + # @return [Hash, nil] The raw payload from the provider + def raw_payload + provider_account.raw_payload + end + + # Returns metadata about this provider and account + # Automatically includes institution metadata if the adapter includes Provider::InstitutionMetadata + # @return [Hash] Metadata hash + def metadata + base_metadata = { + provider_name: provider_name, + provider_type: provider_type + } + + # Include institution metadata if the module is included + if respond_to?(:institution_metadata) + base_metadata.merge!(institution: institution_metadata) + end + + base_metadata + end +end diff --git a/app/models/provider/factory.rb b/app/models/provider/factory.rb new file mode 100644 index 000000000..145963b35 --- /dev/null +++ b/app/models/provider/factory.rb @@ -0,0 +1,66 @@ +class Provider::Factory + class << self + # Register a provider adapter + # @param provider_type [String] The provider account class name (e.g., "PlaidAccount") + # @param adapter_class [Class] The adapter class (e.g., Provider::PlaidAdapter) + def register(provider_type, adapter_class) + registry[provider_type] = adapter_class + end + + # Creates an adapter for a given provider account + # @param provider_account [PlaidAccount, SimplefinAccount] The provider-specific account + # @param account [Account] Optional account reference + # @return [Provider::Base] An adapter instance + def create_adapter(provider_account, account: nil) + return nil if provider_account.nil? + + provider_type = provider_account.class.name + adapter_class = registry[provider_type] + + # If not registered, try to load the adapter + if adapter_class.nil? + ensure_adapters_loaded + adapter_class = registry[provider_type] + end + + raise ArgumentError, "Unknown provider type: #{provider_type}. Did you forget to register it?" unless adapter_class + + adapter_class.new(provider_account, account: account) + end + + # Creates an adapter from an AccountProvider record + # @param account_provider [AccountProvider] The account provider record + # @return [Provider::Base] An adapter instance + def from_account_provider(account_provider) + return nil if account_provider.nil? + + create_adapter(account_provider.provider, account: account_provider.account) + end + + # Get list of registered provider types + # @return [Array] List of registered provider type names + def registered_provider_types + ensure_adapters_loaded + registry.keys + end + + private + + def registry + @registry ||= {} + end + + # Ensures all provider adapters are loaded + # This is needed for Rails autoloading in development/test environments + def ensure_adapters_loaded + return if @adapters_loaded + + # Require all adapter files to trigger registration + Dir[Rails.root.join("app/models/provider/*_adapter.rb")].each do |file| + require_dependency file + end + + @adapters_loaded = true + end + end +end diff --git a/app/models/provider/institution_metadata.rb b/app/models/provider/institution_metadata.rb new file mode 100644 index 000000000..2998d3aa4 --- /dev/null +++ b/app/models/provider/institution_metadata.rb @@ -0,0 +1,40 @@ +# Module for providers that provide institution/bank metadata +# Include this module in your adapter if the provider returns institution information +module Provider::InstitutionMetadata + extend ActiveSupport::Concern + + # Returns the institution's domain (e.g., "chase.com") + # @return [String, nil] The institution domain or nil if not available + def institution_domain + nil + end + + # Returns the institution's display name (e.g., "Chase Bank") + # @return [String, nil] The institution name or nil if not available + def institution_name + nil + end + + # Returns the institution's website URL + # @return [String, nil] The institution URL or nil if not available + def institution_url + nil + end + + # Returns the institution's brand color (for UI purposes) + # @return [String, nil] The hex color code or nil if not available + def institution_color + nil + end + + # Returns a hash of all institution metadata + # @return [Hash] Hash containing institution metadata + def institution_metadata + { + domain: institution_domain, + name: institution_name, + url: institution_url, + color: institution_color + }.compact + end +end diff --git a/app/models/provider/plaid_adapter.rb b/app/models/provider/plaid_adapter.rb new file mode 100644 index 000000000..4503ba8d6 --- /dev/null +++ b/app/models/provider/plaid_adapter.rb @@ -0,0 +1,48 @@ +class Provider::PlaidAdapter < Provider::Base + include Provider::Syncable + include Provider::InstitutionMetadata + + # Register this adapter with the factory + Provider::Factory.register("PlaidAccount", self) + + def provider_name + "plaid" + end + + def sync_path + Rails.application.routes.url_helpers.sync_plaid_item_path(item) + end + + def item + provider_account.plaid_item + end + + def can_delete_holdings? + false + end + + def institution_domain + url_string = item&.institution_url + return nil unless url_string.present? + + begin + uri = URI.parse(url_string) + uri.host&.gsub(/^www\./, "") + rescue URI::InvalidURIError + Rails.logger.warn("Invalid institution URL for Plaid account #{provider_account.id}: #{url_string}") + nil + end + end + + def institution_name + item&.name + end + + def institution_url + item&.institution_url + end + + def institution_color + item&.institution_color + end +end diff --git a/app/models/provider/simplefin_adapter.rb b/app/models/provider/simplefin_adapter.rb new file mode 100644 index 000000000..9cd758347 --- /dev/null +++ b/app/models/provider/simplefin_adapter.rb @@ -0,0 +1,60 @@ +class Provider::SimplefinAdapter < Provider::Base + include Provider::Syncable + include Provider::InstitutionMetadata + + # Register this adapter with the factory + Provider::Factory.register("SimplefinAccount", self) + + def provider_name + "simplefin" + end + + def sync_path + Rails.application.routes.url_helpers.sync_simplefin_item_path(item) + end + + def item + provider_account.simplefin_item + end + + def can_delete_holdings? + false + end + + def institution_domain + org_data = provider_account.org_data + return nil unless org_data.present? + + domain = org_data["domain"] + url = org_data["url"] || org_data["sfin-url"] + + # Derive domain from URL if missing + if domain.blank? && url.present? + begin + domain = URI.parse(url).host&.gsub(/^www\./, "") + rescue URI::InvalidURIError + Rails.logger.warn("Invalid institution URL for SimpleFin account #{provider_account.id}: #{url}") + end + end + + domain + end + + def institution_name + org_data = provider_account.org_data + return nil unless org_data.present? + + org_data["name"] || item&.institution_name + end + + def institution_url + org_data = provider_account.org_data + return nil unless org_data.present? + + org_data["url"] || org_data["sfin-url"] || item&.institution_url + end + + def institution_color + item&.institution_color + end +end diff --git a/app/models/provider/syncable.rb b/app/models/provider/syncable.rb new file mode 100644 index 000000000..918325448 --- /dev/null +++ b/app/models/provider/syncable.rb @@ -0,0 +1,35 @@ +# Module for providers that support syncing with external services +# Include this module in your adapter if the provider supports sync operations +module Provider::Syncable + extend ActiveSupport::Concern + + # Returns the path to sync this provider's item + # @return [String] The sync path + def sync_path + raise NotImplementedError, "#{self.class} must implement #sync_path" + end + + # Returns the provider's item/connection object + # @return [Object] The item object (e.g., PlaidItem, SimplefinItem) + def item + raise NotImplementedError, "#{self.class} must implement #item" + end + + # Check if the item is currently syncing + # @return [Boolean] True if syncing, false otherwise + def syncing? + item&.syncing? || false + end + + # Returns the current sync status + # @return [String, nil] The status string or nil + def status + item&.status + end + + # Check if the item requires an update (e.g., re-authentication) + # @return [Boolean] True if update required, false otherwise + def requires_update? + status == "requires_update" + end +end diff --git a/app/models/simplefin_account.rb b/app/models/simplefin_account.rb index 3b2089c68..8e5000dac 100644 --- a/app/models/simplefin_account.rb +++ b/app/models/simplefin_account.rb @@ -1,11 +1,21 @@ class SimplefinAccount < ApplicationRecord belongs_to :simplefin_item - has_one :account, dependent: :destroy + # Legacy association via foreign key (will be removed after migration) + has_one :account, dependent: :nullify, foreign_key: :simplefin_account_id + + # New association through account_providers + has_one :account_provider, as: :provider, dependent: :destroy + has_one :linked_account, through: :account_provider, source: :account validates :name, :account_type, :currency, presence: true validate :has_balance + # Helper to get account using new system first, falling back to legacy + def current_account + linked_account || account + end + def upsert_simplefin_snapshot!(account_snapshot) # Convert to symbol keys or handle both string and symbol keys snapshot = account_snapshot.with_indifferent_access diff --git a/app/models/simplefin_account/investments/holdings_processor.rb b/app/models/simplefin_account/investments/holdings_processor.rb index 8cf370860..27fec583c 100644 --- a/app/models/simplefin_account/investments/holdings_processor.rb +++ b/app/models/simplefin_account/investments/holdings_processor.rb @@ -17,21 +17,6 @@ class SimplefinAccount::Investments::HoldingsProcessor security = resolve_security(symbol, simplefin_holding["description"]) next unless security.present? - # Use external_id for precise matching - external_id = "simplefin_#{holding_id}" - - # Use the created timestamp as the holding date, fallback to current date - holding_date = parse_holding_date(simplefin_holding["created"]) || Date.current - - holding = account.holdings.find_or_initialize_by( - external_id: external_id - ) do |h| - # Set required fields on initialization - h.security = security - h.date = holding_date - h.currency = simplefin_holding["currency"] || "USD" - end - # Parse all the data SimpleFin provides qty = parse_decimal(simplefin_holding["shares"]) market_value = parse_decimal(simplefin_holding["market_value"]) @@ -44,22 +29,22 @@ class SimplefinAccount::Investments::HoldingsProcessor parse_decimal(simplefin_holding["purchase_price"]) || 0 end - holding.assign_attributes( + # Use the created timestamp as the holding date, fallback to current date + holding_date = parse_holding_date(simplefin_holding["created"]) || Date.current + + import_adapter.import_holding( security: security, - date: holding_date, - currency: simplefin_holding["currency"] || "USD", - qty: qty, - price: price, + quantity: qty, amount: market_value, - cost_basis: cost_basis + currency: simplefin_holding["currency"] || "USD", + date: holding_date, + price: price, + cost_basis: cost_basis, + external_id: "simplefin_#{holding_id}", + account_provider_id: simplefin_account.account_provider&.id, + source: "simplefin", + delete_future_holdings: false # SimpleFin tracks each holding uniquely ) - - ActiveRecord::Base.transaction do - holding.save! - - # With external_id matching, each holding is uniquely tracked - # No need to delete other holdings since each has its own lifecycle - end rescue => e ctx = (defined?(symbol) && symbol.present?) ? " #{symbol}" : "" Rails.logger.error "Error processing SimpleFin holding#{ctx}: #{e.message}" @@ -70,8 +55,12 @@ class SimplefinAccount::Investments::HoldingsProcessor private attr_reader :simplefin_account + def import_adapter + @import_adapter ||= Account::ProviderImportAdapter.new(account) + end + def account - simplefin_account.account + simplefin_account.current_account end def holdings_data diff --git a/app/models/simplefin_account/investments/transactions_processor.rb b/app/models/simplefin_account/investments/transactions_processor.rb index eb76503d1..6644a64dc 100644 --- a/app/models/simplefin_account/investments/transactions_processor.rb +++ b/app/models/simplefin_account/investments/transactions_processor.rb @@ -6,7 +6,7 @@ class SimplefinAccount::Investments::TransactionsProcessor end def process - return unless simplefin_account.account&.accountable_type == "Investment" + return unless simplefin_account.current_account&.accountable_type == "Investment" return unless simplefin_account.raw_transactions_payload.present? transactions_data = simplefin_account.raw_transactions_payload @@ -20,7 +20,7 @@ class SimplefinAccount::Investments::TransactionsProcessor attr_reader :simplefin_account def account - simplefin_account.account + simplefin_account.current_account end def process_investment_transaction(transaction_data) @@ -30,28 +30,23 @@ class SimplefinAccount::Investments::TransactionsProcessor posted_date = parse_date(data[:posted]) external_id = "simplefin_#{data[:id]}" - # Check if entry already exists - existing_entry = Entry.find_by(plaid_id: external_id) - - unless existing_entry - # For investment accounts, create as regular transaction - # In the future, we could detect trade patterns and create Trade entries - transaction = Transaction.new(external_id: external_id) - - Entry.create!( - account: account, - name: data[:description] || "Investment transaction", - amount: amount, - date: posted_date, - currency: account.currency, - entryable: transaction, - plaid_id: external_id - ) - end + # Use the unified import adapter for consistent handling + import_adapter.import_transaction( + external_id: external_id, + amount: amount, + currency: account.currency, + date: posted_date, + name: data[:description] || "Investment transaction", + source: "simplefin" + ) rescue => e Rails.logger.error("Failed to process SimpleFin investment transaction #{data[:id]}: #{e.message}") end + def import_adapter + @import_adapter ||= Account::ProviderImportAdapter.new(account) + end + def parse_amount(amount_value) parsed_amount = case amount_value when String diff --git a/app/models/simplefin_account/liabilities/credit_processor.rb b/app/models/simplefin_account/liabilities/credit_processor.rb index 7534fa7ed..efe061b59 100644 --- a/app/models/simplefin_account/liabilities/credit_processor.rb +++ b/app/models/simplefin_account/liabilities/credit_processor.rb @@ -5,7 +5,7 @@ class SimplefinAccount::Liabilities::CreditProcessor end def process - return unless simplefin_account.account&.accountable_type == "CreditCard" + return unless simplefin_account.current_account&.accountable_type == "CreditCard" # Update credit card specific attributes if available update_credit_attributes @@ -14,18 +14,26 @@ class SimplefinAccount::Liabilities::CreditProcessor private attr_reader :simplefin_account + def import_adapter + @import_adapter ||= Account::ProviderImportAdapter.new(account) + end + def account - simplefin_account.account + simplefin_account.current_account end def update_credit_attributes # SimpleFin provides available_balance which could be credit limit for cards available_balance = simplefin_account.raw_payload&.dig("available-balance") - if available_balance.present? && account.accountable.respond_to?(:available_credit=) + if available_balance.present? credit_limit = parse_decimal(available_balance) - account.accountable.available_credit = credit_limit if credit_limit > 0 - account.accountable.save! + if credit_limit > 0 + import_adapter.update_accountable_attributes( + attributes: { available_credit: credit_limit }, + source: "simplefin" + ) + end end end diff --git a/app/models/simplefin_account/liabilities/loan_processor.rb b/app/models/simplefin_account/liabilities/loan_processor.rb index 7bb8854de..53e1b92f8 100644 --- a/app/models/simplefin_account/liabilities/loan_processor.rb +++ b/app/models/simplefin_account/liabilities/loan_processor.rb @@ -5,7 +5,7 @@ class SimplefinAccount::Liabilities::LoanProcessor end def process - return unless simplefin_account.account&.accountable_type == "Loan" + return unless simplefin_account.current_account&.accountable_type == "Loan" # Update loan specific attributes if available update_loan_attributes @@ -15,7 +15,7 @@ class SimplefinAccount::Liabilities::LoanProcessor attr_reader :simplefin_account def account - simplefin_account.account + simplefin_account.current_account end def update_loan_attributes diff --git a/app/models/simplefin_account/processor.rb b/app/models/simplefin_account/processor.rb index a41575212..f645f33d2 100644 --- a/app/models/simplefin_account/processor.rb +++ b/app/models/simplefin_account/processor.rb @@ -9,7 +9,7 @@ class SimplefinAccount::Processor # Processing the account is the first step and if it fails, we halt # Each subsequent step can fail independently, but we continue processing def process - unless simplefin_account.account.present? + unless simplefin_account.current_account.present? return end @@ -24,13 +24,13 @@ class SimplefinAccount::Processor def process_account! # This should not happen in normal flow since accounts are created manually # during setup, but keeping as safety check - if simplefin_account.account.blank? + if simplefin_account.current_account.blank? Rails.logger.error("SimpleFin account #{simplefin_account.id} has no associated Account - this should not happen after manual setup") return end # Update account balance and cash balance from latest SimpleFin data - account = simplefin_account.account + account = simplefin_account.current_account balance = simplefin_account.current_balance || simplefin_account.available_balance || 0 # SimpleFin returns negative balances for credit cards (liabilities) @@ -60,7 +60,7 @@ class SimplefinAccount::Processor end def process_investments - return unless simplefin_account.account&.accountable_type == "Investment" + return unless simplefin_account.current_account&.accountable_type == "Investment" SimplefinAccount::Investments::TransactionsProcessor.new(simplefin_account).process SimplefinAccount::Investments::HoldingsProcessor.new(simplefin_account).process rescue => e @@ -68,7 +68,7 @@ class SimplefinAccount::Processor end def process_liabilities - case simplefin_account.account&.accountable_type + case simplefin_account.current_account&.accountable_type when "CreditCard" SimplefinAccount::Liabilities::CreditProcessor.new(simplefin_account).process when "Loan" diff --git a/app/models/simplefin_account/transactions/processor.rb b/app/models/simplefin_account/transactions/processor.rb index 16caba2ba..ea4725066 100644 --- a/app/models/simplefin_account/transactions/processor.rb +++ b/app/models/simplefin_account/transactions/processor.rb @@ -37,6 +37,6 @@ class SimplefinAccount::Transactions::Processor end def account - simplefin_account.account + simplefin_account.current_account end end diff --git a/app/models/simplefin_entry/processor.rb b/app/models/simplefin_entry/processor.rb index 51225520c..a9bb606bb 100644 --- a/app/models/simplefin_entry/processor.rb +++ b/app/models/simplefin_entry/processor.rb @@ -6,42 +6,26 @@ class SimplefinEntry::Processor end def process - SimplefinAccount.transaction do - entry = account.entries.find_or_initialize_by(plaid_id: external_id) do |e| - e.entryable = Transaction.new - end - - entry.assign_attributes( - amount: amount, - currency: currency, - date: date - ) - - entry.enrich_attribute( - :name, - name, - source: "simplefin" - ) - - # SimpleFin provides no category data - categories will be set by AI or rules - - if merchant - entry.transaction.enrich_attribute( - :merchant_id, - merchant.id, - source: "simplefin" - ) - end - - entry.save! - end + import_adapter.import_transaction( + external_id: external_id, + amount: amount, + currency: currency, + date: date, + name: name, + source: "simplefin", + merchant: merchant + ) end private attr_reader :simplefin_transaction, :simplefin_account + def import_adapter + @import_adapter ||= Account::ProviderImportAdapter.new(account) + end + def account - simplefin_account.account + simplefin_account.current_account end def data diff --git a/app/views/accounts/_logo.html.erb b/app/views/accounts/_logo.html.erb index 2893b1772..364e7ccb5 100644 --- a/app/views/accounts/_logo.html.erb +++ b/app/views/accounts/_logo.html.erb @@ -7,7 +7,7 @@ "full" => "w-full h-full" } %> -<% if account.plaid_account_id? && account.institution_domain.present? && Setting.brand_fetch_client_id.present? %> +<% if account.linked? && account.institution_domain.present? && Setting.brand_fetch_client_id.present? %> <%= image_tag "https://cdn.brandfetch.io/#{account.institution_domain}/icon/fallback/lettermark/w/40/h/40?c=#{Setting.brand_fetch_client_id}", class: "shrink-0 rounded-full #{size_classes[size]}" %> <% elsif account.logo.attached? %> <%= image_tag account.logo, class: "shrink-0 rounded-full #{size_classes[size]}" %> diff --git a/app/views/accounts/show/_activity.html.erb b/app/views/accounts/show/_activity.html.erb index da5e5ecb1..ce9e74cd8 100644 --- a/app/views/accounts/show/_activity.html.erb +++ b/app/views/accounts/show/_activity.html.erb @@ -4,7 +4,7 @@
<%= tag.h2 t(".title"), class: "font-medium text-lg" %> - <% unless @account.plaid_account_id.present? %> + <% unless @account.linked? %> <%= render DS::Menu.new(variant: "button") do |menu| %> <% menu.with_button(text: "New", variant: "secondary", icon: "plus") %> diff --git a/app/views/holdings/show.html.erb b/app/views/holdings/show.html.erb index 4c1341ea6..9e84a43f0 100644 --- a/app/views/holdings/show.html.erb +++ b/app/views/holdings/show.html.erb @@ -85,7 +85,7 @@
<% end %> - <% unless @holding.account.plaid_account_id.present? %> + <% if @holding.account.can_delete_holdings? %> <% dialog.with_section(title: t(".settings"), open: true) do %>
diff --git a/app/views/simplefin_items/show.html.erb b/app/views/simplefin_items/show.html.erb index eb28bd687..ed6e1c445 100644 --- a/app/views/simplefin_items/show.html.erb +++ b/app/views/simplefin_items/show.html.erb @@ -63,10 +63,10 @@

<%= number_to_currency(simplefin_account.current_balance || 0) %>

- <% if simplefin_account.account %> + <% if simplefin_account.current_account %> <%= render DS::Link.new( text: "View Account", - href: account_path(simplefin_account.account), + href: account_path(simplefin_account.current_account), variant: :outline ) %> <% else %> diff --git a/db/migrate/20251027085448_create_account_providers.rb b/db/migrate/20251027085448_create_account_providers.rb new file mode 100644 index 000000000..8d5d03825 --- /dev/null +++ b/db/migrate/20251027085448_create_account_providers.rb @@ -0,0 +1,15 @@ +class CreateAccountProviders < ActiveRecord::Migration[7.2] + def change + create_table :account_providers, id: :uuid do |t| + t.references :account, type: :uuid, null: false, foreign_key: true, index: true + t.references :provider, type: :uuid, null: false, polymorphic: true, index: true + t.timestamps + end + + # Ensure an account can only have one provider of each type + add_index :account_providers, [ :account_id, :provider_type ], unique: true + + # Ensure a provider can only be linked to one account + add_index :account_providers, [ :provider_type, :provider_id ], unique: true + end +end diff --git a/db/migrate/20251027085614_migrate_account_providers_data.rb b/db/migrate/20251027085614_migrate_account_providers_data.rb new file mode 100644 index 000000000..788249caf --- /dev/null +++ b/db/migrate/20251027085614_migrate_account_providers_data.rb @@ -0,0 +1,36 @@ +class MigrateAccountProvidersData < ActiveRecord::Migration[7.2] + def up + # Migrate Plaid accounts + execute <<-SQL + INSERT INTO account_providers (id, account_id, provider_type, provider_id, created_at, updated_at) + SELECT + gen_random_uuid(), + accounts.id, + 'PlaidAccount', + accounts.plaid_account_id, + NOW(), + NOW() + FROM accounts + WHERE accounts.plaid_account_id IS NOT NULL + SQL + + # Migrate SimpleFin accounts + execute <<-SQL + INSERT INTO account_providers (id, account_id, provider_type, provider_id, created_at, updated_at) + SELECT + gen_random_uuid(), + accounts.id, + 'SimplefinAccount', + accounts.simplefin_account_id, + NOW(), + NOW() + FROM accounts + WHERE accounts.simplefin_account_id IS NOT NULL + SQL + end + + def down + # Delete all account provider records + execute "DELETE FROM account_providers" + end +end diff --git a/db/migrate/20251027110502_add_external_id_and_source_to_entries.rb b/db/migrate/20251027110502_add_external_id_and_source_to_entries.rb new file mode 100644 index 000000000..7ea902874 --- /dev/null +++ b/db/migrate/20251027110502_add_external_id_and_source_to_entries.rb @@ -0,0 +1,9 @@ +class AddExternalIdAndSourceToEntries < ActiveRecord::Migration[7.2] + def change + add_column :entries, :external_id, :string + add_column :entries, :source, :string + + # Add unique index on external_id + source combination to prevent duplicates + add_index :entries, [ :external_id, :source ], unique: true, where: "external_id IS NOT NULL AND source IS NOT NULL" + end +end diff --git a/db/migrate/20251027110609_add_provider_merchant_id_index_to_merchants.rb b/db/migrate/20251027110609_add_provider_merchant_id_index_to_merchants.rb new file mode 100644 index 000000000..ac73ce596 --- /dev/null +++ b/db/migrate/20251027110609_add_provider_merchant_id_index_to_merchants.rb @@ -0,0 +1,9 @@ +class AddProviderMerchantIdIndexToMerchants < ActiveRecord::Migration[7.2] + def change + # Add unique index on provider_merchant_id + source for ProviderMerchant + add_index :merchants, [ :provider_merchant_id, :source ], + unique: true, + where: "provider_merchant_id IS NOT NULL AND type = 'ProviderMerchant'", + name: "index_merchants_on_provider_merchant_id_and_source" + end +end diff --git a/db/migrate/20251027141735_add_account_provider_to_holdings.rb b/db/migrate/20251027141735_add_account_provider_to_holdings.rb new file mode 100644 index 000000000..86ce8989f --- /dev/null +++ b/db/migrate/20251027141735_add_account_provider_to_holdings.rb @@ -0,0 +1,5 @@ +class AddAccountProviderToHoldings < ActiveRecord::Migration[7.2] + def change + add_reference :holdings, :account_provider, null: true, foreign_key: true, type: :uuid, index: true + end +end diff --git a/db/migrate/20251028104241_fix_account_providers_indexes.rb b/db/migrate/20251028104241_fix_account_providers_indexes.rb new file mode 100644 index 000000000..73412fa1d --- /dev/null +++ b/db/migrate/20251028104241_fix_account_providers_indexes.rb @@ -0,0 +1,18 @@ +class FixAccountProvidersIndexes < ActiveRecord::Migration[7.2] + def change + # Remove the overly restrictive unique index on account_id alone + # This was preventing an account from having multiple providers + remove_index :account_providers, name: "index_account_providers_on_account_id" + + # Add proper composite unique index to match model validation + # This allows an account to have multiple providers, but only one of each type + # e.g., Account can have PlaidAccount + SimplefinAccount, but not two PlaidAccounts + add_index :account_providers, [ :account_id, :provider_type ], + unique: true, + name: "index_account_providers_on_account_and_provider_type" + + # Remove redundant non-unique index (cleanup) + # Line 30 already has a unique index on the same columns + remove_index :account_providers, name: "index_account_providers_on_provider" + end +end diff --git a/db/migrate/20251028104851_backfill_entries_external_id_from_plaid_id.rb b/db/migrate/20251028104851_backfill_entries_external_id_from_plaid_id.rb new file mode 100644 index 000000000..ee4f9493b --- /dev/null +++ b/db/migrate/20251028104851_backfill_entries_external_id_from_plaid_id.rb @@ -0,0 +1,26 @@ +class BackfillEntriesExternalIdFromPlaidId < ActiveRecord::Migration[7.2] + def up + # Backfill external_id from plaid_id for entries that have plaid_id but no external_id + # Set source to 'plaid' for these entries as well + execute <<-SQL + UPDATE entries e + SET external_id = e.plaid_id, + source = COALESCE(e.source, 'plaid') + WHERE e.plaid_id IS NOT NULL + AND e.external_id IS NULL + AND (e.source IS NULL OR e.source = 'plaid') + SQL + end + + def down + # Reverse the migration by clearing external_id and source for entries where they match plaid_id + execute <<-SQL + UPDATE entries + SET external_id = NULL, + source = NULL + WHERE plaid_id IS NOT NULL + AND external_id = plaid_id + AND source = 'plaid' + SQL + end +end diff --git a/db/migrate/20251028104916_update_entries_external_id_index.rb b/db/migrate/20251028104916_update_entries_external_id_index.rb new file mode 100644 index 000000000..8672e1751 --- /dev/null +++ b/db/migrate/20251028104916_update_entries_external_id_index.rb @@ -0,0 +1,25 @@ +class UpdateEntriesExternalIdIndex < ActiveRecord::Migration[7.2] + def up + # Remove the old index on [external_id, source] + remove_index :entries, name: "index_entries_on_external_id_and_source", if_exists: true + + # Add new index on [account_id, source, external_id] with WHERE clause + # This ensures external_id is unique per (account, source) combination + # Allows same account to have multiple providers with separate entries + add_index :entries, [ :account_id, :source, :external_id ], + unique: true, + where: "(external_id IS NOT NULL) AND (source IS NOT NULL)", + name: "index_entries_on_account_source_and_external_id" + end + + def down + # Remove the new index + remove_index :entries, name: "index_entries_on_account_source_and_external_id", if_exists: true + + # Restore the old index + add_index :entries, [ :external_id, :source ], + unique: true, + where: "(external_id IS NOT NULL) AND (source IS NOT NULL)", + name: "index_entries_on_external_id_and_source" + end +end diff --git a/db/migrate/20251028174016_remove_duplicate_holdings_index.rb b/db/migrate/20251028174016_remove_duplicate_holdings_index.rb new file mode 100644 index 000000000..f4890ce06 --- /dev/null +++ b/db/migrate/20251028174016_remove_duplicate_holdings_index.rb @@ -0,0 +1,5 @@ +class RemoveDuplicateHoldingsIndex < ActiveRecord::Migration[7.2] + def change + remove_index :holdings, name: "index_holdings_on_account_and_external_id" + end +end diff --git a/db/schema.rb b/db/schema.rb index 5255d298e..eed02506e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_10_25_095800) do +ActiveRecord::Schema[7.2].define(version: 2025_10_28_174016) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -19,6 +19,16 @@ ActiveRecord::Schema[7.2].define(version: 2025_10_25_095800) do # Note that some types may not work with other database engines. Be careful if changing database. create_enum "account_status", ["ok", "syncing", "error"] + create_table "account_providers", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.uuid "account_id", null: false + t.string "provider_type", null: false + t.uuid "provider_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["account_id", "provider_type"], name: "index_account_providers_on_account_and_provider_type", unique: true + t.index ["provider_type", "provider_id"], name: "index_account_providers_on_provider_type_and_provider_id", unique: true + end + create_table "accounts", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.string "subtype" t.uuid "family_id", null: false @@ -29,7 +39,7 @@ ActiveRecord::Schema[7.2].define(version: 2025_10_25_095800) do t.uuid "accountable_id" t.decimal "balance", precision: 19, scale: 4 t.string "currency" - t.virtual "classification", type: :string, as: "\nCASE\n WHEN ((accountable_type)::text = ANY ((ARRAY['Loan'::character varying, 'CreditCard'::character varying, 'OtherLiability'::character varying])::text[])) THEN 'liability'::text\n ELSE 'asset'::text\nEND", stored: true + t.virtual "classification", type: :string, as: "\nCASE\n WHEN ((accountable_type)::text = ANY (ARRAY[('Loan'::character varying)::text, ('CreditCard'::character varying)::text, ('OtherLiability'::character varying)::text])) THEN 'liability'::text\n ELSE 'asset'::text\nEND", stored: true t.uuid "import_id" t.uuid "plaid_account_id" t.decimal "cash_balance", precision: 19, scale: 4, default: "0.0" @@ -238,8 +248,11 @@ ActiveRecord::Schema[7.2].define(version: 2025_10_25_095800) do t.boolean "excluded", default: false t.string "plaid_id" t.jsonb "locked_attributes", default: {} + t.string "external_id" + t.string "source" t.index "lower((name)::text)", name: "index_entries_on_lower_name" t.index ["account_id", "date"], name: "index_entries_on_account_id_and_date" + t.index ["account_id", "source", "external_id"], name: "index_entries_on_account_source_and_external_id", unique: true, where: "((external_id IS NOT NULL) AND (source IS NOT NULL))" t.index ["account_id"], name: "index_entries_on_account_id" t.index ["date"], name: "index_entries_on_date" t.index ["entryable_type"], name: "index_entries_on_entryable_type" @@ -295,10 +308,11 @@ ActiveRecord::Schema[7.2].define(version: 2025_10_25_095800) do t.datetime "updated_at", null: false t.string "external_id" t.decimal "cost_basis", precision: 19, scale: 4 + t.uuid "account_provider_id" t.index ["account_id", "external_id"], name: "idx_holdings_on_account_id_external_id_unique", unique: true, where: "(external_id IS NOT NULL)" - t.index ["account_id", "external_id"], name: "index_holdings_on_account_and_external_id", unique: true t.index ["account_id", "security_id", "date", "currency"], name: "idx_on_account_id_security_id_date_currency_5323e39f8b", unique: true t.index ["account_id"], name: "index_holdings_on_account_id" + t.index ["account_provider_id"], name: "index_holdings_on_account_provider_id" t.index ["security_id"], name: "index_holdings_on_security_id" end @@ -464,6 +478,7 @@ ActiveRecord::Schema[7.2].define(version: 2025_10_25_095800) do t.string "provider_merchant_id" t.index ["family_id", "name"], name: "index_merchants_on_family_id_and_name", unique: true, where: "((type)::text = 'FamilyMerchant'::text)" t.index ["family_id"], name: "index_merchants_on_family_id" + t.index ["provider_merchant_id", "source"], name: "index_merchants_on_provider_merchant_id_and_source", unique: true, where: "((provider_merchant_id IS NOT NULL) AND ((type)::text = 'ProviderMerchant'::text))" t.index ["source", "name"], name: "index_merchants_on_source_and_name", unique: true, where: "((type)::text = 'ProviderMerchant'::text)" t.index ["type"], name: "index_merchants_on_type" end @@ -917,6 +932,7 @@ ActiveRecord::Schema[7.2].define(version: 2025_10_25_095800) do t.string "subtype" end + add_foreign_key "account_providers", "accounts" add_foreign_key "accounts", "families" add_foreign_key "accounts", "imports" add_foreign_key "accounts", "plaid_accounts" @@ -933,6 +949,7 @@ ActiveRecord::Schema[7.2].define(version: 2025_10_25_095800) do add_foreign_key "entries", "accounts" add_foreign_key "entries", "imports" add_foreign_key "family_exports", "families" + add_foreign_key "holdings", "account_providers" add_foreign_key "holdings", "accounts" add_foreign_key "holdings", "securities" add_foreign_key "impersonation_session_logs", "impersonation_sessions" diff --git a/test/controllers/accounts_controller_test.rb b/test/controllers/accounts_controller_test.rb index ec26ef49f..c95d11e7a 100644 --- a/test/controllers/accounts_controller_test.rb +++ b/test/controllers/accounts_controller_test.rb @@ -32,4 +32,29 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest assert_enqueued_with job: DestroyJob assert_equal "Account scheduled for deletion", flash[:notice] end + + test "syncing linked account triggers sync for all provider items" do + plaid_account = plaid_accounts(:one) + plaid_item = plaid_account.plaid_item + AccountProvider.create!(account: @account, provider: plaid_account) + + # Reload to ensure the account has the provider association loaded + @account.reload + + # Mock at the class level since controller loads account from DB + Account.any_instance.expects(:syncing?).returns(false) + PlaidItem.any_instance.expects(:syncing?).returns(false) + PlaidItem.any_instance.expects(:sync_later).once + + post sync_account_url(@account) + assert_redirected_to account_url(@account) + end + + test "syncing unlinked account calls account sync_later" do + Account.any_instance.expects(:syncing?).returns(false) + Account.any_instance.expects(:sync_later).once + + post sync_account_url(@account) + assert_redirected_to account_url(@account) + end end diff --git a/test/controllers/simplefin_items_controller_test.rb b/test/controllers/simplefin_items_controller_test.rb index 26d26475a..1fa99aa8a 100644 --- a/test/controllers/simplefin_items_controller_test.rb +++ b/test/controllers/simplefin_items_controller_test.rb @@ -179,8 +179,8 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest # Verify old SimpleFin accounts no longer reference Maybe accounts old_simplefin_account1.reload old_simplefin_account2.reload - assert_nil old_simplefin_account1.account - assert_nil old_simplefin_account2.account + assert_nil old_simplefin_account1.current_account + assert_nil old_simplefin_account2.current_account # Verify old SimpleFin item is scheduled for deletion @simplefin_item.reload @@ -229,7 +229,7 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest maybe_account.reload old_simplefin_account.reload assert_equal old_simplefin_account.id, maybe_account.simplefin_account_id - assert_equal maybe_account, old_simplefin_account.account + assert_equal maybe_account, old_simplefin_account.current_account # Old item still scheduled for deletion @simplefin_item.reload diff --git a/test/models/account/current_balance_manager_test.rb b/test/models/account/current_balance_manager_test.rb index 0d7b914be..539a4d3c9 100644 --- a/test/models/account/current_balance_manager_test.rb +++ b/test/models/account/current_balance_manager_test.rb @@ -4,6 +4,13 @@ class Account::CurrentBalanceManagerTest < ActiveSupport::TestCase setup do @family = families(:empty) @linked_account = accounts(:connected) + + # Create account_provider to make the account actually linked + # (The fixture has plaid_account but that's the legacy association) + @linked_account.account_providers.find_or_create_by!( + provider_type: "PlaidAccount", + provider_id: plaid_accounts(:one).id + ) end # ------------------------------------------------------------------------------------------------- diff --git a/test/models/account/linkable_test.rb b/test/models/account/linkable_test.rb new file mode 100644 index 000000000..a02453c39 --- /dev/null +++ b/test/models/account/linkable_test.rb @@ -0,0 +1,69 @@ +require "test_helper" + +class Account::LinkableTest < ActiveSupport::TestCase + setup do + @family = families(:dylan_family) + @account = accounts(:depository) + end + + test "linked? returns true when account has providers" do + plaid_account = plaid_accounts(:one) + AccountProvider.create!(account: @account, provider: plaid_account) + + assert @account.linked? + end + + test "linked? returns false when account has no providers" do + assert @account.unlinked? + end + + test "providers returns all provider adapters" do + plaid_account = plaid_accounts(:one) + AccountProvider.create!(account: @account, provider: plaid_account) + + providers = @account.providers + assert_equal 1, providers.count + assert_kind_of Provider::PlaidAdapter, providers.first + end + + test "provider_for returns specific provider adapter" do + plaid_account = plaid_accounts(:one) + AccountProvider.create!(account: @account, provider: plaid_account) + + adapter = @account.provider_for("PlaidAccount") + assert_kind_of Provider::PlaidAdapter, adapter + end + + test "linked_to? checks if account is linked to specific provider type" do + plaid_account = plaid_accounts(:one) + AccountProvider.create!(account: @account, provider: plaid_account) + + assert @account.linked_to?("PlaidAccount") + refute @account.linked_to?("SimplefinAccount") + end + + test "can_delete_holdings? returns true for unlinked accounts" do + assert @account.unlinked? + assert @account.can_delete_holdings? + end + + test "can_delete_holdings? returns false when any provider disallows deletion" do + plaid_account = plaid_accounts(:one) + AccountProvider.create!(account: @account, provider: plaid_account) + + # PlaidAdapter.can_delete_holdings? returns false by default + refute @account.can_delete_holdings? + end + + test "can_delete_holdings? returns true only when all providers allow deletion" do + plaid_account = plaid_accounts(:one) + AccountProvider.create!(account: @account, provider: plaid_account) + + # Stub all providers to return true + @account.providers.each do |provider| + provider.stubs(:can_delete_holdings?).returns(true) + end + + assert @account.can_delete_holdings? + end +end diff --git a/test/models/account/provider_import_adapter_test.rb b/test/models/account/provider_import_adapter_test.rb new file mode 100644 index 000000000..290c6db21 --- /dev/null +++ b/test/models/account/provider_import_adapter_test.rb @@ -0,0 +1,569 @@ +require "test_helper" + +class Account::ProviderImportAdapterTest < ActiveSupport::TestCase + setup do + @account = accounts(:depository) + @adapter = Account::ProviderImportAdapter.new(@account) + @family = families(:dylan_family) + end + + test "imports transaction with all parameters" do + category = categories(:income) + merchant = ProviderMerchant.create!( + provider_merchant_id: "test_merchant_123", + name: "Test Merchant", + source: "plaid" + ) + + assert_difference "@account.entries.count", 1 do + entry = @adapter.import_transaction( + external_id: "plaid_test_123", + amount: 100.50, + currency: "USD", + date: Date.today, + name: "Test Transaction", + source: "plaid", + category_id: category.id, + merchant: merchant + ) + + assert_equal 100.50, entry.amount + assert_equal "USD", entry.currency + assert_equal Date.today, entry.date + assert_equal "Test Transaction", entry.name + assert_equal category.id, entry.transaction.category_id + assert_equal merchant.id, entry.transaction.merchant_id + end + end + + test "imports transaction with minimal parameters" do + assert_difference "@account.entries.count", 1 do + entry = @adapter.import_transaction( + external_id: "simplefin_abc", + amount: 50.00, + currency: "USD", + date: Date.today, + name: "Simple Transaction", + source: "simplefin" + ) + + assert_equal 50.00, entry.amount + assert_equal "simplefin_abc", entry.external_id + assert_equal "simplefin", entry.source + assert_nil entry.transaction.category_id + assert_nil entry.transaction.merchant_id + end + end + + test "updates existing transaction instead of creating duplicate" do + # Create initial transaction + entry = @adapter.import_transaction( + external_id: "plaid_duplicate_test", + amount: 100.00, + currency: "USD", + date: Date.today, + name: "Original Name", + source: "plaid" + ) + + # Import again with different data - should update, not create new + assert_no_difference "@account.entries.count" do + updated_entry = @adapter.import_transaction( + external_id: "plaid_duplicate_test", + amount: 200.00, + currency: "USD", + date: Date.today, + name: "Updated Name", + source: "plaid" + ) + + assert_equal entry.id, updated_entry.id + assert_equal 200.00, updated_entry.amount + assert_equal "Updated Name", updated_entry.name + end + end + + test "allows same external_id from different sources without collision" do + # Create transaction from SimpleFin with ID "transaction_123" + simplefin_entry = @adapter.import_transaction( + external_id: "transaction_123", + amount: 100.00, + currency: "USD", + date: Date.today, + name: "SimpleFin Transaction", + source: "simplefin" + ) + + # Create transaction from Plaid with same ID "transaction_123" - should NOT collide + # because external_id is unique per (account, source) combination + assert_difference "@account.entries.count", 1 do + plaid_entry = @adapter.import_transaction( + external_id: "transaction_123", + amount: 200.00, + currency: "USD", + date: Date.today, + name: "Plaid Transaction", + source: "plaid" + ) + + # Should be different entries + assert_not_equal simplefin_entry.id, plaid_entry.id + assert_equal "simplefin", simplefin_entry.source + assert_equal "plaid", plaid_entry.source + assert_equal "transaction_123", simplefin_entry.external_id + assert_equal "transaction_123", plaid_entry.external_id + end + end + + test "raises error when external_id is missing" do + exception = assert_raises(ArgumentError) do + @adapter.import_transaction( + external_id: "", + amount: 100.00, + currency: "USD", + date: Date.today, + name: "Test", + source: "plaid" + ) + end + + assert_equal "external_id is required", exception.message + end + + test "raises error when source is missing" do + exception = assert_raises(ArgumentError) do + @adapter.import_transaction( + external_id: "test_123", + amount: 100.00, + currency: "USD", + date: Date.today, + name: "Test", + source: "" + ) + end + + assert_equal "source is required", exception.message + end + + test "finds or creates merchant with all data" do + assert_difference "ProviderMerchant.count", 1 do + merchant = @adapter.find_or_create_merchant( + provider_merchant_id: "plaid_merchant_123", + name: "Test Merchant", + source: "plaid", + website_url: "https://example.com", + logo_url: "https://example.com/logo.png" + ) + + assert_equal "Test Merchant", merchant.name + assert_equal "plaid", merchant.source + assert_equal "plaid_merchant_123", merchant.provider_merchant_id + assert_equal "https://example.com", merchant.website_url + assert_equal "https://example.com/logo.png", merchant.logo_url + end + end + + test "returns nil when merchant data is insufficient" do + merchant = @adapter.find_or_create_merchant( + provider_merchant_id: "", + name: "", + source: "plaid" + ) + + assert_nil merchant + end + + test "finds existing merchant instead of creating duplicate" do + existing_merchant = ProviderMerchant.create!( + provider_merchant_id: "existing_123", + name: "Existing Merchant", + source: "plaid" + ) + + assert_no_difference "ProviderMerchant.count" do + merchant = @adapter.find_or_create_merchant( + provider_merchant_id: "existing_123", + name: "Existing Merchant", + source: "plaid" + ) + + assert_equal existing_merchant.id, merchant.id + end + end + + test "updates account balance" do + @adapter.update_balance( + balance: 5000.00, + cash_balance: 4500.00, + source: "plaid" + ) + + @account.reload + assert_equal 5000.00, @account.balance + assert_equal 4500.00, @account.cash_balance + end + + test "updates account balance without cash_balance" do + @adapter.update_balance( + balance: 3000.00, + source: "simplefin" + ) + + @account.reload + assert_equal 3000.00, @account.balance + assert_equal 3000.00, @account.cash_balance + end + + test "imports holding with all parameters" do + investment_account = accounts(:investment) + adapter = Account::ProviderImportAdapter.new(investment_account) + security = securities(:aapl) + + # Use a date that doesn't conflict with fixtures (fixtures use today and 1.day.ago) + holding_date = Date.today - 2.days + + assert_difference "investment_account.holdings.count", 1 do + holding = adapter.import_holding( + security: security, + quantity: 10.5, + amount: 1575.00, + currency: "USD", + date: holding_date, + price: 150.00, + source: "plaid" + ) + + assert_equal security.id, holding.security_id + assert_equal 10.5, holding.qty + assert_equal 1575.00, holding.amount + assert_equal 150.00, holding.price + assert_equal holding_date, holding.date + end + end + + test "raises error when security is missing for holding import" do + exception = assert_raises(ArgumentError) do + @adapter.import_holding( + security: nil, + quantity: 10, + amount: 1000, + currency: "USD", + date: Date.today, + source: "plaid" + ) + end + + assert_equal "security is required", exception.message + end + + test "imports trade with all parameters" do + investment_account = accounts(:investment) + adapter = Account::ProviderImportAdapter.new(investment_account) + security = securities(:aapl) + + assert_difference "investment_account.entries.count", 1 do + entry = adapter.import_trade( + security: security, + quantity: 5, + price: 150.00, + amount: 750.00, + currency: "USD", + date: Date.today, + source: "plaid" + ) + + assert_kind_of Trade, entry.entryable + assert_equal 5, entry.entryable.qty + assert_equal 150.00, entry.entryable.price + assert_equal 750.00, entry.amount + assert_match(/Buy.*5.*shares/i, entry.name) + end + end + + test "raises error when security is missing for trade import" do + exception = assert_raises(ArgumentError) do + @adapter.import_trade( + security: nil, + quantity: 5, + price: 100, + amount: 500, + currency: "USD", + date: Date.today, + source: "plaid" + ) + end + + assert_equal "security is required", exception.message + end + + test "stores account_provider_id when importing holding" do + investment_account = accounts(:investment) + adapter = Account::ProviderImportAdapter.new(investment_account) + security = securities(:aapl) + account_provider = AccountProvider.create!( + account: investment_account, + provider: plaid_accounts(:one) + ) + + holding = adapter.import_holding( + security: security, + quantity: 10, + amount: 1500, + currency: "USD", + date: Date.today, + price: 150, + source: "plaid", + account_provider_id: account_provider.id + ) + + assert_equal account_provider.id, holding.account_provider_id + end + + test "does not delete future holdings when can_delete_holdings? returns false" do + investment_account = accounts(:investment) + adapter = Account::ProviderImportAdapter.new(investment_account) + security = securities(:aapl) + + # Create a future holding + future_holding = investment_account.holdings.create!( + security: security, + qty: 5, + amount: 750, + currency: "USD", + date: Date.today + 1.day, + price: 150 + ) + + # Mock can_delete_holdings? to return false + investment_account.expects(:can_delete_holdings?).returns(false) + + # Import a holding with delete_future_holdings flag + adapter.import_holding( + security: security, + quantity: 10, + amount: 1500, + currency: "USD", + date: Date.today, + price: 150, + source: "plaid", + delete_future_holdings: true + ) + + # Future holding should still exist + assert Holding.exists?(future_holding.id) + end + + test "deletes only holdings from same provider when account_provider_id is provided" do + investment_account = accounts(:investment) + adapter = Account::ProviderImportAdapter.new(investment_account) + security = securities(:aapl) + + # Create an account provider + plaid_account = PlaidAccount.create!( + current_balance: 1000, + available_balance: 1000, + currency: "USD", + name: "Test Plaid Account", + plaid_item: plaid_items(:one), + plaid_id: "acc_mock_test_1", + plaid_type: "investment", + plaid_subtype: "brokerage" + ) + + provider = AccountProvider.create!( + account: investment_account, + provider: plaid_account + ) + + # Create future holdings - one from the provider, one without a provider + future_holding_with_provider = investment_account.holdings.create!( + security: security, + qty: 5, + amount: 750, + currency: "USD", + date: Date.today + 1.day, + price: 150, + account_provider_id: provider.id + ) + + future_holding_without_provider = investment_account.holdings.create!( + security: security, + qty: 3, + amount: 450, + currency: "USD", + date: Date.today + 2.days, + price: 150, + account_provider_id: nil + ) + + # Mock can_delete_holdings? to return true + investment_account.expects(:can_delete_holdings?).returns(true) + + # Import a holding with provider ID and delete_future_holdings flag + adapter.import_holding( + security: security, + quantity: 10, + amount: 1500, + currency: "USD", + date: Date.today, + price: 150, + source: "plaid", + account_provider_id: provider.id, + delete_future_holdings: true + ) + + # Only the holding from the same provider should be deleted + assert_not Holding.exists?(future_holding_with_provider.id) + assert Holding.exists?(future_holding_without_provider.id) + end + + test "deletes all future holdings when account_provider_id is not provided and can_delete_holdings? returns true" do + investment_account = accounts(:investment) + adapter = Account::ProviderImportAdapter.new(investment_account) + security = securities(:aapl) + + # Create two future holdings + future_holding_1 = investment_account.holdings.create!( + security: security, + qty: 5, + amount: 750, + currency: "USD", + date: Date.today + 1.day, + price: 150 + ) + + future_holding_2 = investment_account.holdings.create!( + security: security, + qty: 3, + amount: 450, + currency: "USD", + date: Date.today + 2.days, + price: 150 + ) + + # Mock can_delete_holdings? to return true + investment_account.expects(:can_delete_holdings?).returns(true) + + # Import a holding without account_provider_id + adapter.import_holding( + security: security, + quantity: 10, + amount: 1500, + currency: "USD", + date: Date.today, + price: 150, + source: "plaid", + delete_future_holdings: true + ) + + # All future holdings should be deleted + assert_not Holding.exists?(future_holding_1.id) + assert_not Holding.exists?(future_holding_2.id) + end + + test "updates existing trade attributes instead of keeping stale data" do + investment_account = accounts(:investment) + adapter = Account::ProviderImportAdapter.new(investment_account) + aapl = securities(:aapl) + msft = securities(:msft) + + # Create initial trade + entry = adapter.import_trade( + external_id: "plaid_trade_123", + security: aapl, + quantity: 5, + price: 150.00, + amount: 750.00, + currency: "USD", + date: Date.today, + source: "plaid" + ) + + # Import again with updated attributes - should update Trade, not keep stale data + assert_no_difference "investment_account.entries.count" do + updated_entry = adapter.import_trade( + external_id: "plaid_trade_123", + security: msft, + quantity: 10, + price: 200.00, + amount: 2000.00, + currency: "USD", + date: Date.today, + source: "plaid" + ) + + assert_equal entry.id, updated_entry.id + # Trade attributes should be updated + assert_equal msft.id, updated_entry.entryable.security_id + assert_equal 10, updated_entry.entryable.qty + assert_equal 200.00, updated_entry.entryable.price + assert_equal "USD", updated_entry.entryable.currency + # Entry attributes should also be updated + assert_equal 2000.00, updated_entry.amount + end + end + + test "raises error when external_id collision occurs across different entryable types for transaction" do + investment_account = accounts(:investment) + adapter = Account::ProviderImportAdapter.new(investment_account) + security = securities(:aapl) + + # Create a trade with external_id "collision_test" + adapter.import_trade( + external_id: "collision_test", + security: security, + quantity: 5, + price: 150.00, + amount: 750.00, + currency: "USD", + date: Date.today, + source: "plaid" + ) + + # Try to create a transaction with the same external_id and source + exception = assert_raises(ArgumentError) do + adapter.import_transaction( + external_id: "collision_test", + amount: 100.00, + currency: "USD", + date: Date.today, + name: "Test Transaction", + source: "plaid" + ) + end + + assert_match(/Entry with external_id.*already exists with different entryable type/i, exception.message) + end + + test "raises error when external_id collision occurs across different entryable types for trade" do + investment_account = accounts(:investment) + adapter = Account::ProviderImportAdapter.new(investment_account) + security = securities(:aapl) + + # Create a transaction with external_id "collision_test_2" + adapter.import_transaction( + external_id: "collision_test_2", + amount: 100.00, + currency: "USD", + date: Date.today, + name: "Test Transaction", + source: "plaid" + ) + + # Try to create a trade with the same external_id and source + exception = assert_raises(ArgumentError) do + adapter.import_trade( + external_id: "collision_test_2", + security: security, + quantity: 5, + price: 150.00, + amount: 750.00, + currency: "USD", + date: Date.today, + source: "plaid" + ) + end + + assert_match(/Entry with external_id.*already exists with different entryable type/i, exception.message) + end +end diff --git a/test/models/account_provider_test.rb b/test/models/account_provider_test.rb new file mode 100644 index 000000000..6e656df36 --- /dev/null +++ b/test/models/account_provider_test.rb @@ -0,0 +1,132 @@ +require "test_helper" + +class AccountProviderTest < ActiveSupport::TestCase + setup do + @account = accounts(:depository) + @family = families(:dylan_family) + + # Create provider items + @plaid_item = PlaidItem.create!( + family: @family, + plaid_id: "test_plaid_item", + access_token: "test_token", + name: "Test Bank" + ) + + @simplefin_item = SimplefinItem.create!( + family: @family, + name: "Test SimpleFin Bank", + access_url: "https://example.com/access" + ) + + # Create provider accounts + @plaid_account = PlaidAccount.create!( + plaid_item: @plaid_item, + name: "Plaid Checking", + plaid_id: "plaid_123", + plaid_type: "depository", + currency: "USD", + current_balance: 1000 + ) + + @simplefin_account = SimplefinAccount.create!( + simplefin_item: @simplefin_item, + name: "SimpleFin Checking", + account_id: "sf_123", + account_type: "checking", + currency: "USD", + current_balance: 2000 + ) + end + + test "allows an account to have multiple different provider types" do + # Should be able to link both Plaid and SimpleFin to same account + plaid_provider = AccountProvider.create!( + account: @account, + provider: @plaid_account + ) + + simplefin_provider = AccountProvider.create!( + account: @account, + provider: @simplefin_account + ) + + assert_equal 2, @account.account_providers.count + assert_includes @account.account_providers, plaid_provider + assert_includes @account.account_providers, simplefin_provider + end + + test "prevents duplicate provider type for same account" do + # Create first PlaidAccount link + AccountProvider.create!( + account: @account, + provider: @plaid_account + ) + + # Create another PlaidAccount + another_plaid_account = PlaidAccount.create!( + plaid_item: @plaid_item, + name: "Another Plaid Account", + plaid_id: "plaid_456", + plaid_type: "savings", + currency: "USD", + current_balance: 5000 + ) + + # Should not be able to link another PlaidAccount to same account + duplicate_provider = AccountProvider.new( + account: @account, + provider: another_plaid_account + ) + + assert_not duplicate_provider.valid? + assert_includes duplicate_provider.errors[:account_id], "has already been taken" + end + + test "prevents same provider account from linking to multiple accounts" do + # Link provider to first account + AccountProvider.create!( + account: @account, + provider: @plaid_account + ) + + # Try to link same provider to another account + another_account = accounts(:investment) + + duplicate_link = AccountProvider.new( + account: another_account, + provider: @plaid_account + ) + + assert_not duplicate_link.valid? + assert_includes duplicate_link.errors[:provider_id], "has already been taken" + end + + test "adapter method returns correct adapter" do + provider = AccountProvider.create!( + account: @account, + provider: @plaid_account + ) + + adapter = provider.adapter + + assert_kind_of Provider::PlaidAdapter, adapter + assert_equal "plaid", adapter.provider_name + assert_equal @account, adapter.account + end + + test "provider_name delegates to adapter" do + plaid_provider = AccountProvider.create!( + account: @account, + provider: @plaid_account + ) + + simplefin_provider = AccountProvider.create!( + account: accounts(:investment), + provider: @simplefin_account + ) + + assert_equal "plaid", plaid_provider.provider_name + assert_equal "simplefin", simplefin_provider.provider_name + end +end diff --git a/test/models/plaid_account/investments/holdings_processor_test.rb b/test/models/plaid_account/investments/holdings_processor_test.rb index 3aa797dad..169784ae9 100644 --- a/test/models/plaid_account/investments/holdings_processor_test.rb +++ b/test/models/plaid_account/investments/holdings_processor_test.rb @@ -55,7 +55,7 @@ class PlaidAccount::Investments::HoldingsProcessorTest < ActiveSupport::TestCase processor.process end - holdings = Holding.where(account: @plaid_account.account).order(:date) + holdings = Holding.where(account: @plaid_account.current_account).order(:date) assert_equal 100, holdings.first.qty assert_equal 100, holdings.first.price @@ -70,31 +70,36 @@ class PlaidAccount::Investments::HoldingsProcessorTest < ActiveSupport::TestCase assert_equal Date.current, holdings.second.date end - # When Plaid provides holdings data, it includes an "institution_price_as_of" date - # which represents when the holdings were last updated. Any holdings in our database - # after this date are now stale and should be deleted, as the Plaid data is the - # authoritative source of truth for the current holdings. - test "deletes stale holdings per security based on institution price date" do - account = @plaid_account.account + # Plaid does not delete future holdings because it doesn't support holdings deletion + # (PlaidAdapter#can_delete_holdings? returns false). This test verifies that future + # holdings are NOT deleted when processing Plaid holdings data. + test "does not delete future holdings when processing Plaid holdings" do + account = @plaid_account.current_account + + # Create account_provider + account_provider = AccountProvider.create!( + account: account, + provider: @plaid_account + ) # Create a third security for testing third_security = Security.create!(ticker: "GOOGL", name: "Google", exchange_operating_mic: "XNAS", country_code: "US") - # Scenario 3: AAPL has a stale holding that should be deleted - stale_aapl_holding = account.holdings.create!( + # Create a future AAPL holding that should NOT be deleted + future_aapl_holding = account.holdings.create!( security: securities(:aapl), date: Date.current, qty: 80, price: 180, amount: 14400, - currency: "USD" + currency: "USD", + account_provider_id: account_provider.id ) - # Plaid returns 3 holdings with different scenarios + # Plaid returns holdings from yesterday - future holdings should remain test_investments_payload = { securities: [], holdings: [ - # Scenario 1: Current date holding (no deletions needed) { "security_id" => "current", "quantity" => 50, @@ -102,7 +107,6 @@ class PlaidAccount::Investments::HoldingsProcessorTest < ActiveSupport::TestCase "iso_currency_code" => "USD", "institution_price_as_of" => Date.current }, - # Scenario 2: Yesterday's holding with no future holdings { "security_id" => "clean", "quantity" => 75, @@ -110,9 +114,8 @@ class PlaidAccount::Investments::HoldingsProcessorTest < ActiveSupport::TestCase "iso_currency_code" => "USD", "institution_price_as_of" => 1.day.ago.to_date }, - # Scenario 3: Yesterday's holding with stale future holding { - "security_id" => "stale", + "security_id" => "past", "quantity" => 100, "institution_price" => 100, "iso_currency_code" => "USD", @@ -134,17 +137,17 @@ class PlaidAccount::Investments::HoldingsProcessorTest < ActiveSupport::TestCase .returns(OpenStruct.new(security: third_security, cash_equivalent?: false, brokerage_cash?: false)) @security_resolver.expects(:resolve) - .with(plaid_security_id: "stale") + .with(plaid_security_id: "past") .returns(OpenStruct.new(security: securities(:aapl), cash_equivalent?: false, brokerage_cash?: false)) processor = PlaidAccount::Investments::HoldingsProcessor.new(@plaid_account, security_resolver: @security_resolver) processor.process - # Should have created 3 new holdings - assert_equal 3, account.holdings.count + # Should have created 3 new holdings PLUS the existing future holding (total 4) + assert_equal 4, account.holdings.count - # Scenario 3: Should have deleted the stale AAPL holding - assert_not account.holdings.exists?(stale_aapl_holding.id) + # Future AAPL holding should still exist (NOT deleted) + assert account.holdings.exists?(future_aapl_holding.id) # Should have the correct holdings from Plaid assert account.holdings.exists?(security: securities(:msft), date: Date.current, qty: 50) @@ -192,6 +195,134 @@ class PlaidAccount::Investments::HoldingsProcessorTest < ActiveSupport::TestCase end # Should have created the successful holding - assert @plaid_account.account.holdings.exists?(security: securities(:aapl), qty: 200) + assert @plaid_account.current_account.holdings.exists?(security: securities(:aapl), qty: 200) + end + + test "handles string values and computes amount using BigDecimal arithmetic" do + test_investments_payload = { + securities: [], + holdings: [ + { + "security_id" => "string_values", + "quantity" => "10.5", + "institution_price" => "150.75", + "iso_currency_code" => "USD", + "institution_price_as_of" => "2025-01-15" + } + ], + transactions: [] + } + + @plaid_account.update!(raw_investments_payload: test_investments_payload) + + @security_resolver.expects(:resolve) + .with(plaid_security_id: "string_values") + .returns(OpenStruct.new(security: securities(:aapl))) + + processor = PlaidAccount::Investments::HoldingsProcessor.new(@plaid_account, security_resolver: @security_resolver) + + assert_difference "Holding.count", 1 do + processor.process + end + + holding = @plaid_account.current_account.holdings.find_by( + security: securities(:aapl), + date: Date.parse("2025-01-15"), + currency: "USD" + ) + + assert_not_nil holding, "Expected to find holding for AAPL on 2025-01-15" + assert_equal BigDecimal("10.5"), holding.qty + assert_equal BigDecimal("150.75"), holding.price + assert_equal BigDecimal("1582.875"), holding.amount # 10.5 * 150.75 using BigDecimal + assert_equal Date.parse("2025-01-15"), holding.date + end + + test "skips holdings with nil quantity or price" do + test_investments_payload = { + securities: [], + holdings: [ + { + "security_id" => "missing_quantity", + "quantity" => nil, + "institution_price" => 100, + "iso_currency_code" => "USD" + }, + { + "security_id" => "missing_price", + "quantity" => 100, + "institution_price" => nil, + "iso_currency_code" => "USD" + }, + { + "security_id" => "valid", + "quantity" => 50, + "institution_price" => 50, + "iso_currency_code" => "USD" + } + ], + transactions: [] + } + + @plaid_account.update!(raw_investments_payload: test_investments_payload) + + @security_resolver.expects(:resolve) + .with(plaid_security_id: "missing_quantity") + .returns(OpenStruct.new(security: securities(:aapl))) + + @security_resolver.expects(:resolve) + .with(plaid_security_id: "missing_price") + .returns(OpenStruct.new(security: securities(:msft))) + + @security_resolver.expects(:resolve) + .with(plaid_security_id: "valid") + .returns(OpenStruct.new(security: securities(:aapl))) + + processor = PlaidAccount::Investments::HoldingsProcessor.new(@plaid_account, security_resolver: @security_resolver) + + # Should create only 1 holding (the valid one) + assert_difference "Holding.count", 1 do + processor.process + end + + # Should have created only the valid holding + assert @plaid_account.current_account.holdings.exists?(security: securities(:aapl), qty: 50, price: 50) + assert_not @plaid_account.current_account.holdings.exists?(security: securities(:msft)) + end + + test "uses account currency as fallback when Plaid omits iso_currency_code" do + account = @plaid_account.current_account + + # Ensure the account has a currency + account.update!(currency: "EUR") + + test_investments_payload = { + securities: [], + holdings: [ + { + "security_id" => "no_currency", + "quantity" => 100, + "institution_price" => 100, + "iso_currency_code" => nil, # Plaid omits currency + "institution_price_as_of" => Date.current + } + ], + transactions: [] + } + + @plaid_account.update!(raw_investments_payload: test_investments_payload) + + @security_resolver.expects(:resolve) + .with(plaid_security_id: "no_currency") + .returns(OpenStruct.new(security: securities(:aapl))) + + processor = PlaidAccount::Investments::HoldingsProcessor.new(@plaid_account, security_resolver: @security_resolver) + + assert_difference "Holding.count", 1 do + processor.process + end + + holding = account.holdings.find_by(security: securities(:aapl)) + assert_equal "EUR", holding.currency # Should use account's currency end end diff --git a/test/models/plaid_account/investments/transactions_processor_test.rb b/test/models/plaid_account/investments/transactions_processor_test.rb index 2eb044f1d..be589c513 100644 --- a/test/models/plaid_account/investments/transactions_processor_test.rb +++ b/test/models/plaid_account/investments/transactions_processor_test.rb @@ -10,7 +10,7 @@ class PlaidAccount::Investments::TransactionsProcessorTest < ActiveSupport::Test test_investments_payload = { transactions: [ { - "transaction_id" => "123", + "investment_transaction_id" => "123", "security_id" => "123", "type" => "buy", "quantity" => 1, # Positive, so "buy 1 share" @@ -47,7 +47,7 @@ class PlaidAccount::Investments::TransactionsProcessorTest < ActiveSupport::Test test_investments_payload = { transactions: [ { - "transaction_id" => "123", + "investment_transaction_id" => "cash_123", "type" => "cash", "subtype" => "withdrawal", "amount" => 100, # Positive, so moving money OUT of the account @@ -80,7 +80,7 @@ class PlaidAccount::Investments::TransactionsProcessorTest < ActiveSupport::Test test_investments_payload = { transactions: [ { - "transaction_id" => "123", + "investment_transaction_id" => "fee_123", "type" => "fee", "subtype" => "miscellaneous fee", "amount" => 10.25, @@ -113,7 +113,8 @@ class PlaidAccount::Investments::TransactionsProcessorTest < ActiveSupport::Test test_investments_payload = { transactions: [ { - "transaction_id" => "123", + "investment_transaction_id" => "123", + "security_id" => "123", "type" => "sell", # Correct type "subtype" => "sell", # Correct subtype "quantity" => 1, # ***Incorrect signage***, this should be negative diff --git a/test/models/plaid_account/liabilities/credit_processor_test.rb b/test/models/plaid_account/liabilities/credit_processor_test.rb index d51e79db5..04b100051 100644 --- a/test/models/plaid_account/liabilities/credit_processor_test.rb +++ b/test/models/plaid_account/liabilities/credit_processor_test.rb @@ -8,7 +8,7 @@ class PlaidAccount::Liabilities::CreditProcessorTest < ActiveSupport::TestCase plaid_subtype: "credit_card" ) - @plaid_account.account.update!( + @plaid_account.current_account.update!( accountable: CreditCard.new, ) end @@ -24,8 +24,8 @@ class PlaidAccount::Liabilities::CreditProcessorTest < ActiveSupport::TestCase processor = PlaidAccount::Liabilities::CreditProcessor.new(@plaid_account) processor.process - assert_equal 100, @plaid_account.account.credit_card.minimum_payment - assert_equal 15.0, @plaid_account.account.credit_card.apr + assert_equal 100, @plaid_account.current_account.credit_card.minimum_payment + assert_equal 15.0, @plaid_account.current_account.credit_card.apr end test "does nothing when liability data absent" do @@ -33,7 +33,7 @@ class PlaidAccount::Liabilities::CreditProcessorTest < ActiveSupport::TestCase processor = PlaidAccount::Liabilities::CreditProcessor.new(@plaid_account) processor.process - assert_nil @plaid_account.account.credit_card.minimum_payment - assert_nil @plaid_account.account.credit_card.apr + assert_nil @plaid_account.current_account.credit_card.minimum_payment + assert_nil @plaid_account.current_account.credit_card.apr end end diff --git a/test/models/plaid_account/liabilities/mortgage_processor_test.rb b/test/models/plaid_account/liabilities/mortgage_processor_test.rb index feb9ce8c7..cafa4efde 100644 --- a/test/models/plaid_account/liabilities/mortgage_processor_test.rb +++ b/test/models/plaid_account/liabilities/mortgage_processor_test.rb @@ -8,7 +8,7 @@ class PlaidAccount::Liabilities::MortgageProcessorTest < ActiveSupport::TestCase plaid_subtype: "mortgage" ) - @plaid_account.account.update!(accountable: Loan.new) + @plaid_account.current_account.update!(accountable: Loan.new) end test "updates loan interest rate and type from Plaid data" do @@ -24,7 +24,7 @@ class PlaidAccount::Liabilities::MortgageProcessorTest < ActiveSupport::TestCase processor = PlaidAccount::Liabilities::MortgageProcessor.new(@plaid_account) processor.process - loan = @plaid_account.account.loan + loan = @plaid_account.current_account.loan assert_equal "fixed", loan.rate_type assert_equal 4.25, loan.interest_rate @@ -36,7 +36,7 @@ class PlaidAccount::Liabilities::MortgageProcessorTest < ActiveSupport::TestCase processor = PlaidAccount::Liabilities::MortgageProcessor.new(@plaid_account) processor.process - loan = @plaid_account.account.loan + loan = @plaid_account.current_account.loan assert_nil loan.rate_type assert_nil loan.interest_rate diff --git a/test/models/plaid_account/liabilities/student_loan_processor_test.rb b/test/models/plaid_account/liabilities/student_loan_processor_test.rb index 40fa5b238..53819044f 100644 --- a/test/models/plaid_account/liabilities/student_loan_processor_test.rb +++ b/test/models/plaid_account/liabilities/student_loan_processor_test.rb @@ -9,7 +9,7 @@ class PlaidAccount::Liabilities::StudentLoanProcessorTest < ActiveSupport::TestC ) # Change the underlying accountable to a Loan so the helper method `loan` is available - @plaid_account.account.update!(accountable: Loan.new) + @plaid_account.current_account.update!(accountable: Loan.new) end test "updates loan details including term months from Plaid data" do @@ -25,7 +25,7 @@ class PlaidAccount::Liabilities::StudentLoanProcessorTest < ActiveSupport::TestC processor = PlaidAccount::Liabilities::StudentLoanProcessor.new(@plaid_account) processor.process - loan = @plaid_account.account.loan + loan = @plaid_account.current_account.loan assert_equal "fixed", loan.rate_type assert_equal 5.5, loan.interest_rate @@ -46,7 +46,7 @@ class PlaidAccount::Liabilities::StudentLoanProcessorTest < ActiveSupport::TestC processor = PlaidAccount::Liabilities::StudentLoanProcessor.new(@plaid_account) processor.process - loan = @plaid_account.account.loan + loan = @plaid_account.current_account.loan assert_nil loan.term_months assert_equal 4.8, loan.interest_rate @@ -59,7 +59,7 @@ class PlaidAccount::Liabilities::StudentLoanProcessorTest < ActiveSupport::TestC processor = PlaidAccount::Liabilities::StudentLoanProcessor.new(@plaid_account) processor.process - loan = @plaid_account.account.loan + loan = @plaid_account.current_account.loan assert_nil loan.interest_rate assert_nil loan.initial_balance diff --git a/test/models/plaid_account/processor_test.rb b/test/models/plaid_account/processor_test.rb index 6d6a4af74..fffbe2625 100644 --- a/test/models/plaid_account/processor_test.rb +++ b/test/models/plaid_account/processor_test.rb @@ -29,31 +29,36 @@ class PlaidAccount::ProcessorTest < ActiveSupport::TestCase account = Account.order(created_at: :desc).first assert_equal "Test Plaid Account", account.name - assert_equal @plaid_account.id, account.plaid_account_id assert_equal "checking", account.subtype assert_equal 1000, account.balance assert_equal 1000, account.cash_balance assert_equal "USD", account.currency assert_equal "Depository", account.accountable_type assert_equal "checking", account.subtype + + # Verify AccountProvider was created + assert account.linked? + assert_equal 1, account.account_providers.count + assert_equal @plaid_account.id, account.account_providers.first.provider_id + assert_equal "PlaidAccount", account.account_providers.first.provider_type end test "processing is idempotent with updates and enrichments" do expect_default_subprocessor_calls - assert_equal "Plaid Depository Account", @plaid_account.account.name - assert_equal "checking", @plaid_account.account.subtype + assert_equal "Plaid Depository Account", @plaid_account.current_account.name + assert_equal "checking", @plaid_account.current_account.subtype - @plaid_account.account.update!( + @plaid_account.current_account.update!( name: "User updated name", balance: 2000 # User cannot override balance. This will be overridden by the processor on next processing ) - @plaid_account.account.accountable.update!(subtype: "savings") + @plaid_account.current_account.accountable.update!(subtype: "savings") - @plaid_account.account.lock_attr!(:name) - @plaid_account.account.accountable.lock_attr!(:subtype) - @plaid_account.account.lock_attr!(:balance) # Even if balance somehow becomes locked, Plaid ignores it and overrides it + @plaid_account.current_account.lock_attr!(:name) + @plaid_account.current_account.accountable.lock_attr!(:subtype) + @plaid_account.current_account.lock_attr!(:balance) # Even if balance somehow becomes locked, Plaid ignores it and overrides it assert_no_difference "Account.count" do PlaidAccount::Processor.new(@plaid_account).process @@ -61,9 +66,9 @@ class PlaidAccount::ProcessorTest < ActiveSupport::TestCase @plaid_account.reload - assert_equal "User updated name", @plaid_account.account.name - assert_equal "savings", @plaid_account.account.subtype - assert_equal @plaid_account.current_balance, @plaid_account.account.balance # Overriden by processor + assert_equal "User updated name", @plaid_account.current_account.name + assert_equal "savings", @plaid_account.current_account.subtype + assert_equal @plaid_account.current_balance, @plaid_account.current_account.balance # Overriden by processor end test "account processing failure halts further processing" do @@ -102,7 +107,7 @@ class PlaidAccount::ProcessorTest < ActiveSupport::TestCase PlaidAccount::Processor.new(@plaid_account).process # Verify that the balance was set correctly - account = @plaid_account.account + account = @plaid_account.current_account assert_equal 1000, account.balance assert_equal 1000, account.cash_balance @@ -196,7 +201,7 @@ class PlaidAccount::ProcessorTest < ActiveSupport::TestCase expect_default_subprocessor_calls PlaidAccount::Processor.new(@plaid_account).process - account = @plaid_account.account + account = @plaid_account.current_account original_anchor = account.valuations.current_anchor.first assert_not_nil original_anchor original_anchor_id = original_anchor.id diff --git a/test/models/plaid_account/transactions/processor_test.rb b/test/models/plaid_account/transactions/processor_test.rb index 85272b115..82076a1fb 100644 --- a/test/models/plaid_account/transactions/processor_test.rb +++ b/test/models/plaid_account/transactions/processor_test.rb @@ -37,7 +37,7 @@ class PlaidAccount::Transactions::ProcessorTest < ActiveSupport::TestCase test "removes transactions no longer in plaid" do destroyable_transaction_id = "destroy_me" - @plaid_account.account.entries.create!( + @plaid_account.current_account.entries.create!( plaid_id: destroyable_transaction_id, date: Date.current, amount: 100, diff --git a/test/models/plaid_entry/processor_test.rb b/test/models/plaid_entry/processor_test.rb index bce448b09..730be2e7f 100644 --- a/test/models/plaid_entry/processor_test.rb +++ b/test/models/plaid_entry/processor_test.rb @@ -61,8 +61,9 @@ class PlaidEntry::ProcessorTest < ActiveSupport::TestCase @category_matcher.expects(:match).with("Food").returns(categories(:food_and_drink)) # Create an existing entry - @plaid_account.account.entries.create!( - plaid_id: existing_plaid_id, + @plaid_account.current_account.entries.create!( + external_id: existing_plaid_id, + source: "plaid", amount: 100, currency: "USD", date: Date.current, diff --git a/test/models/provider/plaid_adapter_test.rb b/test/models/provider/plaid_adapter_test.rb new file mode 100644 index 000000000..08a3eda55 --- /dev/null +++ b/test/models/provider/plaid_adapter_test.rb @@ -0,0 +1,41 @@ +require "test_helper" + +class Provider::PlaidAdapterTest < ActiveSupport::TestCase + include ProviderAdapterTestInterface + + setup do + @plaid_account = plaid_accounts(:one) + @account = accounts(:depository) + @adapter = Provider::PlaidAdapter.new(@plaid_account, account: @account) + end + + def adapter + @adapter + end + + # Run shared interface tests + test_provider_adapter_interface + test_syncable_interface + test_institution_metadata_interface + + # Provider-specific tests + test "returns correct provider name" do + assert_equal "plaid", @adapter.provider_name + end + + test "returns correct provider type" do + assert_equal "PlaidAccount", @adapter.provider_type + end + + test "returns plaid item" do + assert_equal @plaid_account.plaid_item, @adapter.item + end + + test "returns account" do + assert_equal @account, @adapter.account + end + + test "can_delete_holdings? returns false" do + assert_equal false, @adapter.can_delete_holdings? + end +end diff --git a/test/models/provider/registry_test.rb b/test/models/provider/registry_test.rb index a0bfb33be..5083a4b4a 100644 --- a/test/models/provider/registry_test.rb +++ b/test/models/provider/registry_test.rb @@ -44,19 +44,22 @@ class Provider::RegistryTest < ActiveSupport::TestCase end test "openai provider falls back to Setting when ENV is empty string" do - # Simulate ENV being set to empty string (common in Docker/env files) - ENV.stubs(:[]).with("OPENAI_ACCESS_TOKEN").returns("") - ENV.stubs(:[]).with("OPENAI_URI_BASE").returns("") - ENV.stubs(:[]).with("OPENAI_MODEL").returns("") + # Mock ENV to return empty string (common in Docker/env files) + # Use stub_env helper which properly stubs ENV access + ClimateControl.modify( + "OPENAI_ACCESS_TOKEN" => "", + "OPENAI_URI_BASE" => "", + "OPENAI_MODEL" => "" + ) do + Setting.stubs(:openai_access_token).returns("test-token-from-setting") + Setting.stubs(:openai_uri_base).returns(nil) + Setting.stubs(:openai_model).returns(nil) - Setting.stubs(:openai_access_token).returns("test-token-from-setting") - Setting.stubs(:openai_uri_base).returns(nil) - Setting.stubs(:openai_model).returns(nil) + provider = Provider::Registry.get_provider(:openai) - provider = Provider::Registry.get_provider(:openai) - - # Should successfully create provider using Setting value - assert_not_nil provider - assert_instance_of Provider::Openai, provider + # Should successfully create provider using Setting value + assert_not_nil provider + assert_instance_of Provider::Openai, provider + end end end diff --git a/test/models/provider/simplefin_adapter_test.rb b/test/models/provider/simplefin_adapter_test.rb new file mode 100644 index 000000000..e97e9f32d --- /dev/null +++ b/test/models/provider/simplefin_adapter_test.rb @@ -0,0 +1,72 @@ +require "test_helper" + +class Provider::SimplefinAdapterTest < ActiveSupport::TestCase + include ProviderAdapterTestInterface + + setup do + @family = families(:dylan_family) + @simplefin_item = SimplefinItem.create!( + family: @family, + name: "Test SimpleFin Bank", + access_url: "https://example.com/access_token" + ) + @simplefin_account = SimplefinAccount.create!( + simplefin_item: @simplefin_item, + name: "SimpleFin Depository Account", + account_id: "sf_mock_1", + account_type: "checking", + currency: "USD", + current_balance: 1000, + available_balance: 1000, + org_data: { + "name" => "SimpleFin Test Bank", + "domain" => "testbank.com", + "url" => "https://testbank.com" + } + ) + @account = accounts(:depository) + @adapter = Provider::SimplefinAdapter.new(@simplefin_account, account: @account) + end + + def adapter + @adapter + end + + # Run shared interface tests + test_provider_adapter_interface + test_syncable_interface + test_institution_metadata_interface + + # Provider-specific tests + test "returns correct provider name" do + assert_equal "simplefin", @adapter.provider_name + end + + test "returns correct provider type" do + assert_equal "SimplefinAccount", @adapter.provider_type + end + + test "returns simplefin item" do + assert_equal @simplefin_account.simplefin_item, @adapter.item + end + + test "returns account" do + assert_equal @account, @adapter.account + end + + test "can_delete_holdings? returns false" do + assert_equal false, @adapter.can_delete_holdings? + end + + test "parses institution domain from org_data" do + assert_equal "testbank.com", @adapter.institution_domain + end + + test "parses institution name from org_data" do + assert_equal "SimpleFin Test Bank", @adapter.institution_name + end + + test "parses institution url from org_data" do + assert_equal "https://testbank.com", @adapter.institution_url + end +end diff --git a/test/support/provider_adapter_test_interface.rb b/test/support/provider_adapter_test_interface.rb new file mode 100644 index 000000000..a5280136e --- /dev/null +++ b/test/support/provider_adapter_test_interface.rb @@ -0,0 +1,140 @@ +# Shared test interface for all provider adapters +# Include this module in your provider adapter test to ensure it implements the required interface +# +# Usage: +# class Provider::AcmeAdapterTest < ActiveSupport::TestCase +# include ProviderAdapterTestInterface +# +# setup do +# @adapter = Provider::AcmeAdapter.new(...) +# end +# +# def adapter +# @adapter +# end +# +# test_provider_adapter_interface +# end +module ProviderAdapterTestInterface + extend ActiveSupport::Concern + + class_methods do + # Tests the core provider adapter interface + # Call this method in your test class to run all interface tests + def test_provider_adapter_interface + test "adapter implements provider_name" do + assert_respond_to adapter, :provider_name + assert_kind_of String, adapter.provider_name + assert adapter.provider_name.present?, "provider_name should not be blank" + end + + test "adapter implements provider_type" do + assert_respond_to adapter, :provider_type + assert_kind_of String, adapter.provider_type + assert adapter.provider_type.present?, "provider_type should not be blank" + end + + test "adapter implements can_delete_holdings?" do + assert_respond_to adapter, :can_delete_holdings? + assert_includes [ true, false ], adapter.can_delete_holdings? + end + + test "adapter implements metadata" do + assert_respond_to adapter, :metadata + metadata = adapter.metadata + + assert_kind_of Hash, metadata + assert_includes metadata.keys, :provider_name + assert_includes metadata.keys, :provider_type + + assert_equal adapter.provider_name, metadata[:provider_name] + assert_equal adapter.provider_type, metadata[:provider_type] + end + + test "adapter implements raw_payload" do + assert_respond_to adapter, :raw_payload + # raw_payload can be nil or a Hash + assert adapter.raw_payload.nil? || adapter.raw_payload.is_a?(Hash) + end + + test "adapter is registered with factory" do + provider_type = adapter.provider_type + assert_includes Provider::Factory.registered_provider_types, provider_type, + "#{provider_type} should be registered with Provider::Factory" + end + end + + # Tests for adapters that include Provider::Syncable + def test_syncable_interface + test "syncable adapter implements sync_path" do + assert_respond_to adapter, :sync_path + assert_kind_of String, adapter.sync_path + assert adapter.sync_path.present?, "sync_path should not be blank" + end + + test "syncable adapter implements item" do + assert_respond_to adapter, :item + assert_not_nil adapter.item, "item should not be nil for syncable providers" + end + + test "syncable adapter implements syncing?" do + assert_respond_to adapter, :syncing? + assert_includes [ true, false ], adapter.syncing? + end + + test "syncable adapter implements status" do + assert_respond_to adapter, :status + # status can be nil or a String + assert adapter.status.nil? || adapter.status.is_a?(String) + end + + test "syncable adapter implements requires_update?" do + assert_respond_to adapter, :requires_update? + assert_includes [ true, false ], adapter.requires_update? + end + end + + # Tests for adapters that include Provider::InstitutionMetadata + def test_institution_metadata_interface + test "institution metadata adapter implements institution_domain" do + assert_respond_to adapter, :institution_domain + # Can be nil or String + assert adapter.institution_domain.nil? || adapter.institution_domain.is_a?(String) + end + + test "institution metadata adapter implements institution_name" do + assert_respond_to adapter, :institution_name + # Can be nil or String + assert adapter.institution_name.nil? || adapter.institution_name.is_a?(String) + end + + test "institution metadata adapter implements institution_url" do + assert_respond_to adapter, :institution_url + # Can be nil or String + assert adapter.institution_url.nil? || adapter.institution_url.is_a?(String) + end + + test "institution metadata adapter implements institution_color" do + assert_respond_to adapter, :institution_color + # Can be nil or String + assert adapter.institution_color.nil? || adapter.institution_color.is_a?(String) + end + + test "institution metadata adapter implements institution_metadata" do + assert_respond_to adapter, :institution_metadata + metadata = adapter.institution_metadata + + assert_kind_of Hash, metadata + # Metadata should only contain non-nil values + metadata.each do |key, value| + assert_not_nil value, "#{key} in institution_metadata should not be nil (it should be omitted instead)" + end + end + end + end + + # Override this method in your test to provide the adapter instance + def adapter + raise NotImplementedError, "Test must implement #adapter method" + end +end