Merge pull request #367 from luckyPipewrench/improved-holdings

Materialize holdings and refresh post‑import balances (credit/investment)

test/jobs/simplefin_holdings_apply_job_test.rb

@@ -0,0 +1,63 @@
+require "test_helper"
+
+class SimplefinHoldingsApplyJobTest < ActiveSupport::TestCase
+  setup do
+    @family = families(:dylan_family)
+    @item = SimplefinItem.create!(family: @family, name: "SF", access_url: "https://example.com/x")
+
+    @account = accounts(:investment)
+
+    # Link SFA to existing investment account via legacy association for simplicity
+    @sfa = @item.simplefin_accounts.create!(
+      name: "Invest",
+      account_id: "sf_invest_1",
+      currency: "USD",
+      account_type: "investment",
+      current_balance: 10_000
+    )
+    @account.update!(simplefin_account_id: @sfa.id)
+  end
+
+  test "materializes holdings from raw_holdings_payload and is idempotent" do
+    # Two holdings: one AAPL (existing security), one NEWCO (should be created)
+    @sfa.update!(
+      raw_holdings_payload: [
+        {
+          "id" => "h1",
+          "symbol" => "AAPL",
+          "quantity" => 10,
+          "market_value" => 2000,
+          "currency" => "USD",
+          "as_of" => (Date.current - 2.days).to_s
+        },
+        {
+          "id" => "h2",
+          "symbol" => "NEWCO",
+          "quantity" => 5,
+          "market_value" => 500,
+          "currency" => "USD",
+          "as_of" => Date.current.to_s
+        }
+      ]
+    )
+
+    assert_difference "Holding.where(account: @account).count", 2 do
+      SimplefinHoldingsApplyJob.perform_now(@sfa.id)
+    end
+
+
+    # Running again should not create duplicates (external_id uniqueness)
+    assert_no_difference "Holding.where(account: @account).count" do
+      SimplefinHoldingsApplyJob.perform_now(@sfa.id)
+    end
+
+    holdings = @account.holdings.order(:external_id)
+    aapl = holdings.find { |h| h.security.ticker == "AAPL" }
+    refute_nil aapl
+    assert_equal 10, aapl.qty
+    assert_equal Money.new(2000, "USD"), aapl.amount_money
+
+    newco_sec = Security.find_by(ticker: "NEWCO")
+    refute_nil newco_sec, "should create NEWCO security via resolver when missing"
+  end
+end

test/models/account/provider_import_adapter_cross_provider_test.rb

@@ -0,0 +1,95 @@
+require "test_helper"
+
+# Ensures a provider cannot "claim" another provider's holdings when external_id lookup misses
+# and a (security, date, currency) match exists. The fallback path must be scoped by account_provider_id.
+class Account::ProviderImportAdapterCrossProviderTest < ActiveSupport::TestCase
+  test "does not claim holdings from a different provider when external_id is present and fallback kicks in" do
+    investment_account = accounts(:investment)
+    adapter = Account::ProviderImportAdapter.new(investment_account)
+    security = securities(:aapl)
+
+    # Create two different account providers for the SAME account
+    # Provider A (e.g., Plaid)
+    ap_a = AccountProvider.create!(
+      account: investment_account,
+      provider: plaid_accounts(:one)
+    )
+
+    # Provider B (e.g., SimpleFin)
+    item = SimplefinItem.create!(family: families(:dylan_family), name: "SF Conn", access_url: "https://example.com/access")
+    sfa_b = SimplefinAccount.create!(
+      simplefin_item: item,
+      name: "SF Invest",
+      account_id: "sf_inv_cross_provider",
+      currency: "USD",
+      account_type: "investment",
+      current_balance: 1000
+    )
+    ap_b = AccountProvider.create!(
+      account: investment_account,
+      provider: sfa_b
+    )
+
+    # Use a date that will not collide with existing fixture holdings for this account
+    holding_date = Date.today - 3.days
+
+    # Existing holding created by Provider A for (security, date, currency)
+    existing_a = investment_account.holdings.create!(
+      security: security,
+      date: holding_date,
+      qty: 1,
+      price: 100,
+      amount: 100,
+      currency: "USD",
+      account_provider_id: ap_a.id
+    )
+
+    # Now import for Provider B with an external_id that doesn't exist yet.
+    # Fallback should NOT "claim" Provider A's row because account_provider_id differs.
+    # Attempt import for Provider B with a conflicting composite key.
+    # Policy: do NOT create a duplicate row and do NOT claim Provider A's row.
+    assert_no_difference "investment_account.holdings.count" do
+      @result_b = adapter.import_holding(
+        security: security,
+        quantity: 2,
+        amount: 220,
+        currency: "USD",
+        date: holding_date,
+        price: 110,
+        cost_basis: nil,
+        external_id: "ext-b-1",
+        source: "simplefin",
+        account_provider_id: ap_b.id,
+        delete_future_holdings: false
+      )
+    end
+
+    # Provider A's holding remains unclaimed (no external_id added) and still owned by A
+    existing_a.reload
+    assert_nil existing_a.external_id
+    assert_equal ap_a.id, existing_a.account_provider_id
+
+    # Adapter returns the existing A row for transparency
+    assert_equal existing_a.id, @result_b.id
+
+    # Idempotency: importing again with the same external_id should not create another row
+    assert_no_difference "investment_account.holdings.count" do
+      again = adapter.import_holding(
+        security: security,
+        quantity: 2,
+        amount: 220,
+        currency: "USD",
+        date: holding_date,
+        price: 110,
+        cost_basis: nil,
+        external_id: "ext-b-1",
+        source: "simplefin",
+        account_provider_id: ap_b.id,
+        delete_future_holdings: false
+      )
+      assert_equal existing_a.id, again.id
+      # Ensure external_id was NOT attached to A's row (no cross-provider claim)
+      assert_nil existing_a.reload.external_id
+    end
+  end
+end

test/models/account/provider_import_adapter_unowned_adoption_test.rb

@@ -0,0 +1,90 @@
+require "test_helper"
+
+# Ensures an "unowned" composite row (no account_provider_id) is fully adopted on
+# first collision: attributes are updated, external_id attached, and
+# account_provider_id set to the importing provider.
+class Account::ProviderImportAdapterUnownedAdoptionTest < ActiveSupport::TestCase
+  test "adopts unowned holding on unique-index collision by updating attrs and provider ownership" do
+    investment_account = accounts(:investment)
+    adapter = Account::ProviderImportAdapter.new(investment_account)
+    security = securities(:aapl)
+
+    # Create a SimpleFin provider for this account (the importer)
+    item = SimplefinItem.create!(family: families(:dylan_family), name: "SF Conn", access_url: "https://example.com/access")
+    sfa = SimplefinAccount.create!(
+      simplefin_item: item,
+      name: "SF Invest",
+      account_id: "sf_inv_unowned_claim",
+      currency: "USD",
+      account_type: "investment",
+      current_balance: 1000
+    )
+    ap = AccountProvider.create!(account: investment_account, provider: sfa)
+
+    holding_date = Date.today - 4.days
+
+    # Existing composite row without provider ownership (unowned)
+    existing_unowned = investment_account.holdings.create!(
+      security: security,
+      date: holding_date,
+      qty: 1,
+      price: 100,
+      amount: 100,
+      currency: "USD",
+      account_provider_id: nil
+    )
+
+    # Import for SimpleFin with an external_id that will collide on composite key
+    # Adapter should NOT create a new row, but should update the existing one:
+    # - qty/price/amount/cost_basis updated
+    # - external_id attached
+    # - account_provider_id adopted to ap.id
+    assert_no_difference "investment_account.holdings.count" do
+      @result = adapter.import_holding(
+        security: security,
+        quantity: 2,
+        amount: 220,
+        currency: "USD",
+        date: holding_date,
+        price: 110,
+        cost_basis: nil,
+        external_id: "ext-unowned-1",
+        source: "simplefin",
+        account_provider_id: ap.id,
+        delete_future_holdings: false
+      )
+    end
+
+    existing_unowned.reload
+
+    # Attributes updated
+    assert_equal 2, existing_unowned.qty
+    assert_equal 110, existing_unowned.price
+    assert_equal 220, existing_unowned.amount
+
+    # Ownership and external_id adopted
+    assert_equal ap.id, existing_unowned.account_provider_id
+    assert_equal "ext-unowned-1", existing_unowned.external_id
+
+    # Adapter returns the same row
+    assert_equal existing_unowned.id, @result.id
+
+    # Idempotency: re-import should not create a duplicate and should return the same row
+    assert_no_difference "investment_account.holdings.count" do
+      again = adapter.import_holding(
+        security: security,
+        quantity: 2,
+        amount: 220,
+        currency: "USD",
+        date: holding_date,
+        price: 110,
+        cost_basis: nil,
+        external_id: "ext-unowned-1",
+        source: "simplefin",
+        account_provider_id: ap.id,
+        delete_future_holdings: false
+      )
+      assert_equal existing_unowned.id, again.id
+    end
+  end
+end

test/models/simplefin_item/importer_post_import_test.rb

@@ -0,0 +1,74 @@
+require "test_helper"
+
+class SimplefinItem::ImporterPostImportTest < ActiveSupport::TestCase
+  setup do
+    @family = families(:dylan_family)
+    @item = SimplefinItem.create!(family: @family, name: "SF Conn", access_url: "https://example.com/access")
+    @sync = Sync.create!(syncable: @item)
+  end
+
+  test "credit account import updates available_credit when available-balance provided" do
+    credit_acct = accounts(:credit_card)
+
+    sfa = @item.simplefin_accounts.create!(
+      name: "CC",
+      account_id: "sf_cc_1",
+      currency: "USD",
+      account_type: "credit",
+      available_balance: 0
+    )
+    # Link via legacy association
+    credit_acct.update!(simplefin_account_id: sfa.id)
+
+    importer = SimplefinItem::Importer.new(@item, simplefin_provider: mock(), sync: @sync)
+
+    account_data = {
+      id: sfa.account_id,
+      name: "CC",
+      balance: -1200.0, # liabilities often negative from provider
+      currency: "USD",
+      "available-balance": 5000.0
+    }
+
+    # Call private method for focused unit test
+    importer.send(:import_account, account_data)
+
+    assert_equal 5000.0, credit_acct.reload.credit_card.available_credit
+  end
+
+  test "investment import recalculates cash_balance when holdings payload changes" do
+    invest_acct = accounts(:investment)
+
+    sfa = @item.simplefin_accounts.create!(
+      name: "Invest",
+      account_id: "sf_inv_1",
+      currency: "USD",
+      account_type: "investment",
+      current_balance: 0
+    )
+    invest_acct.update!(simplefin_account_id: sfa.id)
+
+    importer = SimplefinItem::Importer.new(@item, simplefin_provider: mock(), sync: @sync)
+
+    holdings = [
+      { "id" => "h1", "symbol" => "AAPL", "quantity" => 10, "market_value" => 2000, "currency" => "USD", "as_of" => Date.current.to_s },
+      { "id" => "h2", "symbol" => "MSFT", "quantity" => 20, "market_value" => 4000, "currency" => "USD", "as_of" => Date.current.to_s }
+    ]
+
+    account_data = {
+      id: sfa.account_id,
+      name: "Invest",
+      balance: 10000.0,
+      currency: "USD",
+      holdings: holdings
+    }
+
+    # Prevent the job from running in this unit test; we only care about cash balance recompute
+    SimplefinHoldingsApplyJob.expects(:perform_later).once
+
+    importer.send(:import_account, account_data)
+
+    # Cash balance should be total balance (10_000) minus market_value sum (6_000) = 4_000
+    assert_equal 4000.0, invest_acct.reload.cash_balance
+  end
+end