diff --git a/app/controllers/enable_banking_items_controller.rb b/app/controllers/enable_banking_items_controller.rb index ae69543e7..d2e341387 100644 --- a/app/controllers/enable_banking_items_controller.rb +++ b/app/controllers/enable_banking_items_controller.rb @@ -269,13 +269,17 @@ class EnableBankingItemsController < ApplicationController end # Create the internal Account (uses save! internally, will raise on failure) + # Skip initial sync - provider sync will handle balance creation with correct currency account = Account.create_and_sync( - family: Current.family, - name: enable_banking_account.name, - balance: enable_banking_account.current_balance || 0, - currency: enable_banking_account.currency || "EUR", - accountable_type: accountable_type, - accountable_attributes: {} + { + family: Current.family, + name: enable_banking_account.name, + balance: enable_banking_account.current_balance || 0, + currency: enable_banking_account.currency || "EUR", + accountable_type: accountable_type, + accountable_attributes: {} + }, + skip_initial_sync: true ) # Link account to enable_banking_account via account_providers diff --git a/app/controllers/lunchflow_items_controller.rb b/app/controllers/lunchflow_items_controller.rb index c16cd20ef..2208090e7 100644 --- a/app/controllers/lunchflow_items_controller.rb +++ b/app/controllers/lunchflow_items_controller.rb @@ -182,13 +182,18 @@ class LunchflowItemsController < ApplicationController end # Create the internal Account with proper balance initialization + # Use lunchflow_account.currency (already parsed) and skip initial sync + # because the provider sync will set the correct currency from the balance API account = Account.create_and_sync( - family: Current.family, - name: account_data[:name], - balance: 0, # Initial balance will be set during sync - currency: account_data[:currency] || "USD", - accountable_type: accountable_type, - accountable_attributes: {} + { + family: Current.family, + name: account_data[:name], + balance: 0, # Initial balance will be set during sync + currency: lunchflow_account.currency || "USD", + accountable_type: accountable_type, + accountable_attributes: {} + }, + skip_initial_sync: true ) # Link account to lunchflow_account via account_providers join table @@ -605,13 +610,17 @@ class LunchflowItemsController < ApplicationController selected_subtype = "credit_card" if selected_type == "CreditCard" && selected_subtype.blank? # Create account with user-selected type and subtype (raises on failure) + # Skip initial sync - provider sync will handle balance creation with correct currency account = Account.create_and_sync( - family: Current.family, - name: lunchflow_account.name, - balance: lunchflow_account.current_balance || 0, - currency: lunchflow_account.currency || "USD", - accountable_type: selected_type, - accountable_attributes: selected_subtype.present? ? { subtype: selected_subtype } : {} + { + family: Current.family, + name: lunchflow_account.name, + balance: lunchflow_account.current_balance || 0, + currency: lunchflow_account.currency || "USD", + accountable_type: selected_type, + accountable_attributes: selected_subtype.present? ? { subtype: selected_subtype } : {} + }, + skip_initial_sync: true ) # Link account to lunchflow_account via account_providers join table (raises on failure) diff --git a/app/models/account.rb b/app/models/account.rb index 2e12b7072..b861302c4 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -68,7 +68,7 @@ class Account < ApplicationRecord end class << self - def create_and_sync(attributes) + def create_and_sync(attributes, skip_initial_sync: false) attributes[:accountable_attributes] ||= {} # Ensure accountable is created, even if empty account = new(attributes.merge(cash_balance: attributes[:balance])) initial_balance = attributes.dig(:accountable_attributes, :initial_balance)&.to_d @@ -81,7 +81,9 @@ class Account < ApplicationRecord raise result.error if result.error end - account.sync_later + # Skip initial sync for linked accounts - the provider sync will handle balance creation + # after the correct currency is known + account.sync_later unless skip_initial_sync account end @@ -130,7 +132,8 @@ class Account < ApplicationRecord simplefin_account_id: simplefin_account.id } - create_and_sync(attributes) + # Skip initial sync - provider sync will handle balance creation with correct currency + create_and_sync(attributes, skip_initial_sync: true) end def create_from_enable_banking_account(enable_banking_account, account_type, subtype = nil) @@ -156,11 +159,13 @@ class Account < ApplicationRecord accountable_attributes = {} accountable_attributes[:subtype] = subtype if subtype.present? + # Skip initial sync - provider sync will handle balance creation with correct currency create_and_sync( attributes.merge( accountable_type: account_type, accountable_attributes: accountable_attributes - ) + ), + skip_initial_sync: true ) end diff --git a/app/models/balance/chart_series_builder.rb b/app/models/balance/chart_series_builder.rb index dad108171..c988651b7 100644 --- a/app/models/balance/chart_series_builder.rb +++ b/app/models/balance/chart_series_builder.rb @@ -130,6 +130,7 @@ class Balance::ChartSeriesBuilder b.flows_factor FROM balances b WHERE b.account_id = accounts.id + AND b.currency = accounts.currency AND b.date <= d.date ORDER BY b.date DESC LIMIT 1 diff --git a/db/migrate/20260108000000_cleanup_orphaned_currency_balances.rb b/db/migrate/20260108000000_cleanup_orphaned_currency_balances.rb new file mode 100644 index 000000000..c481723aa --- /dev/null +++ b/db/migrate/20260108000000_cleanup_orphaned_currency_balances.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +# This migration cleans up orphaned balance records that have a currency different +# from their account's current currency. This can happen when linked accounts +# (SimpleFIN, Lunchflow, Enable Banking, Plaid) were created with an initial sync +# before the correct currency was known from the provider. +# +# The fix in Account.create_and_sync with skip_initial_sync: true prevents this +# going forward, but existing data needs to be cleaned up. +class CleanupOrphanedCurrencyBalances < ActiveRecord::Migration[7.2] + def up + # First, identify affected accounts for logging + affected_accounts = execute(<<~SQL).to_a + SELECT DISTINCT + a.id, + a.name, + a.currency as account_currency, + b.currency as orphaned_currency, + COUNT(b.id) as orphaned_balance_count + FROM accounts a + JOIN balances b ON a.id = b.account_id + WHERE b.currency != a.currency + AND ( + a.simplefin_account_id IS NOT NULL + OR a.plaid_account_id IS NOT NULL + OR EXISTS (SELECT 1 FROM account_providers WHERE account_id = a.id) + ) + GROUP BY a.id, a.name, a.currency, b.currency + ORDER BY a.name + SQL + + if affected_accounts.any? + say "Found #{affected_accounts.size} account-currency combinations with orphaned balances:" + affected_accounts.each do |row| + say " - #{row['name']}: #{row['orphaned_balance_count']} balances in #{row['orphaned_currency']} (account is #{row['account_currency']})" + end + + # Delete orphaned balances where currency doesn't match account currency + # Only for linked accounts (provider-connected accounts) + deleted_result = execute(<<~SQL) + DELETE FROM balances + WHERE id IN ( + SELECT b.id + FROM balances b + JOIN accounts a ON b.account_id = a.id + WHERE b.currency != a.currency + AND ( + a.simplefin_account_id IS NOT NULL + OR a.plaid_account_id IS NOT NULL + OR EXISTS (SELECT 1 FROM account_providers WHERE account_id = a.id) + ) + ) + SQL + + say "Deleted orphaned balances from linked accounts" + + # Get unique account IDs that need re-sync + account_ids = affected_accounts.map { |row| row["id"] }.uniq + + say "Scheduling re-sync for #{account_ids.size} affected accounts..." + + # Schedule re-sync for affected accounts to regenerate correct balances + # Use find_each to handle large datasets efficiently + Account.where(id: account_ids).find_each do |account| + account.sync_later + end + + say "Scheduled re-sync for #{account_ids.size} affected accounts" + else + say "No orphaned currency balances found - database is clean" + end + end + + def down + say "This migration cannot be fully reversed." + say "The deleted balances will be regenerated by the scheduled syncs." + say "If syncs haven't run yet, you may need to manually trigger them." + end +end diff --git a/db/schema.rb b/db/schema.rb index 158cd3851..90a40f52a 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: 2026_01_06_152346) do +ActiveRecord::Schema[7.2].define(version: 2026_01_08_131034) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" diff --git a/test/models/account_test.rb b/test/models/account_test.rb index 3733fc700..911b0bfa4 100644 --- a/test/models/account_test.rb +++ b/test/models/account_test.rb @@ -14,6 +14,64 @@ class AccountTest < ActiveSupport::TestCase end end + test "create_and_sync calls sync_later by default" do + Account.any_instance.expects(:sync_later).once + + account = Account.create_and_sync({ + family: @family, + name: "Test Account", + balance: 100, + currency: "USD", + accountable_type: "Depository", + accountable_attributes: {} + }) + + assert account.persisted? + assert_equal "USD", account.currency + assert_equal 100, account.balance + end + + test "create_and_sync skips sync_later when skip_initial_sync is true" do + Account.any_instance.expects(:sync_later).never + + account = Account.create_and_sync( + { + family: @family, + name: "Linked Account", + balance: 500, + currency: "EUR", + accountable_type: "Depository", + accountable_attributes: {} + }, + skip_initial_sync: true + ) + + assert account.persisted? + assert_equal "EUR", account.currency + assert_equal 500, account.balance + end + + test "create_and_sync creates opening anchor with correct currency" do + Account.any_instance.stubs(:sync_later) + + account = Account.create_and_sync( + { + family: @family, + name: "Test Account", + balance: 1000, + currency: "GBP", + accountable_type: "Depository", + accountable_attributes: {} + }, + skip_initial_sync: true + ) + + opening_anchor = account.valuations.opening_anchor.first + assert_not_nil opening_anchor + assert_equal "GBP", opening_anchor.entry.currency + assert_equal 1000, opening_anchor.entry.amount + end + test "gets short/long subtype label" do investment = Investment.new(subtype: "hsa") account = @family.accounts.create!( diff --git a/test/models/balance/chart_series_builder_test.rb b/test/models/balance/chart_series_builder_test.rb index fbf5019fd..b80d51bca 100644 --- a/test/models/balance/chart_series_builder_test.rb +++ b/test/models/balance/chart_series_builder_test.rb @@ -127,4 +127,197 @@ class Balance::ChartSeriesBuilderTest < ActiveSupport::TestCase assert_equal expected, builder.balance_series.map { |v| v.value.amount } end + + test "uses balances matching account currency for correct chart data" do + # This test verifies that chart data is built from balances with proper currency. + # Data integrity is maintained by: + # 1. Account.create_and_sync with skip_initial_sync: true for linked accounts + # 2. Migration cleanup_orphaned_currency_balances for existing data + account = accounts(:depository) + account.balances.destroy_all + + # Account is in USD, create balances in USD + create_balance(account: account, date: 2.days.ago.to_date, balance: 1000) + create_balance(account: account, date: 1.day.ago.to_date, balance: 1500) + create_balance(account: account, date: Date.current, balance: 2000) + + builder = Balance::ChartSeriesBuilder.new( + account_ids: [ account.id ], + currency: "USD", + period: Period.custom(start_date: 2.days.ago.to_date, end_date: Date.current), + interval: "1 day" + ) + + series = builder.balance_series + assert_equal 3, series.size + assert_equal [ 1000, 1500, 2000 ], series.map { |v| v.value.amount } + end + + test "balances are converted to target currency using exchange rates" do + # Create account with EUR currency + family = families(:dylan_family) + account = family.accounts.create!( + name: "EUR Account", + balance: 1000, + currency: "EUR", + accountable: Depository.new + ) + + account.balances.destroy_all + + # Create balances in EUR (matching account currency) + create_balance(account: account, date: 1.day.ago.to_date, balance: 1000) + create_balance(account: account, date: Date.current, balance: 1200) + + # Add exchange rate EUR -> USD + ExchangeRate.create!(date: 1.day.ago.to_date, from_currency: "EUR", to_currency: "USD", rate: 1.1) + + # Request chart in USD (different from account's EUR) + builder = Balance::ChartSeriesBuilder.new( + account_ids: [ account.id ], + currency: "USD", + period: Period.custom(start_date: 1.day.ago.to_date, end_date: Date.current), + interval: "1 day" + ) + + series = builder.balance_series + # EUR balances converted to USD at 1.1 rate (LOCF for today) + assert_equal [ 1100, 1320 ], series.map { |v| v.value.amount } + end + + test "linked account with orphaned currency balances shows correct values after cleanup" do + # This test reproduces the original bug scenario: + # 1. Linked account created with initial sync before correct currency was known + # 2. Opening anchor and first sync created balances with wrong currency (USD) + # 3. Provider sync updated account to correct currency (EUR) and created new balances + # 4. Both USD and EUR balances existed - charts showed wrong values + # + # The fix: + # 1. skip_initial_sync prevents this going forward + # 2. Migration cleans up orphaned balances for existing linked accounts + + # Use the connected (linked) account fixture + linked_account = accounts(:connected) + linked_account.balances.destroy_all + + # Simulate the bug: account is now EUR but has old USD balances from initial sync + linked_account.update!(currency: "EUR") + + # Create orphaned balances in wrong currency (USD) - from initial sync before currency was known + Balance.create!( + account: linked_account, + date: 3.days.ago.to_date, + balance: 1000, + cash_balance: 1000, + currency: "USD", # Wrong currency! + start_cash_balance: 1000, + start_non_cash_balance: 0, + cash_inflows: 0, + cash_outflows: 0, + non_cash_inflows: 0, + non_cash_outflows: 0, + net_market_flows: 0, + cash_adjustments: 0, + non_cash_adjustments: 0, + flows_factor: 1 + ) + + Balance.create!( + account: linked_account, + date: 2.days.ago.to_date, + balance: 1100, + cash_balance: 1100, + currency: "USD", # Wrong currency! + start_cash_balance: 1100, + start_non_cash_balance: 0, + cash_inflows: 0, + cash_outflows: 0, + non_cash_inflows: 0, + non_cash_outflows: 0, + net_market_flows: 0, + cash_adjustments: 0, + non_cash_adjustments: 0, + flows_factor: 1 + ) + + # Create correct balances in EUR - from provider sync after currency was known + create_balance(account: linked_account, date: 1.day.ago.to_date, balance: 5000) + create_balance(account: linked_account, date: Date.current, balance: 5500) + + # Verify we have both currency balances (the bug state) + assert_equal 2, linked_account.balances.where(currency: "USD").count + assert_equal 2, linked_account.balances.where(currency: "EUR").count + + # Simulate migration cleanup: delete orphaned balances with wrong currency + linked_account.balances.where.not(currency: linked_account.currency).delete_all + + # Verify cleanup removed orphaned balances + assert_equal 0, linked_account.balances.where(currency: "USD").count + assert_equal 2, linked_account.balances.where(currency: "EUR").count + + # Now chart should show correct EUR values + builder = Balance::ChartSeriesBuilder.new( + account_ids: [ linked_account.id ], + currency: "EUR", + period: Period.custom(start_date: 2.days.ago.to_date, end_date: Date.current), + interval: "1 day" + ) + + series = builder.balance_series + # After cleanup: only EUR balances exist, chart shows correct values + # Day 2 ago: 0 (no EUR balance), Day 1 ago: 5000, Today: 5500 + assert_equal [ 0, 5000, 5500 ], series.map { |v| v.value.amount } + end + + test "chart ignores orphaned currency balances via currency filter" do + # This test verifies the currency filter correctly ignores orphaned balances. + # The filter `b.currency = accounts.currency` ensures only valid balances are used. + # + # Bug scenario: Account currency changed from USD to EUR after initial sync, + # leaving orphaned USD balances. Without the filter, charts would show wrong values. + + linked_account = accounts(:connected) + linked_account.balances.destroy_all + + # Account is EUR but has orphaned USD balances (bug state) + linked_account.update!(currency: "EUR") + + # Create orphaned USD balance (wrong currency) + Balance.create!( + account: linked_account, + date: 1.day.ago.to_date, + balance: 9999, + cash_balance: 9999, + currency: "USD", # Wrong currency - doesn't match account.currency (EUR) + start_cash_balance: 9999, + start_non_cash_balance: 0, + cash_inflows: 0, + cash_outflows: 0, + non_cash_inflows: 0, + non_cash_outflows: 0, + net_market_flows: 0, + cash_adjustments: 0, + non_cash_adjustments: 0, + flows_factor: 1 + ) + + # Chart correctly ignores USD balance because account.currency is EUR + builder = Balance::ChartSeriesBuilder.new( + account_ids: [ linked_account.id ], + currency: "EUR", + period: Period.custom(start_date: 1.day.ago.to_date, end_date: Date.current), + interval: "1 day" + ) + + series = builder.balance_series + + # Currency filter ensures orphaned USD balance (9999) is ignored + # Chart shows zeros because no EUR balances exist + assert_equal 2, series.size + assert_equal [ 0, 0 ], series.map { |v| v.value.amount } + + # Verify the orphaned balance still exists in DB (migration will clean it up) + assert_equal 1, linked_account.balances.where(currency: "USD").count + assert_equal 0, linked_account.balances.where(currency: "EUR").count + end end