Fix Plaid cash balance double counting (#2222)

* Fix Plaid cash balance double counting

* Fix today's cash balance

* Simplify balance trends in activity view

app/models/balance/reverse_calculator.rb

@@ -21,7 +21,13 @@ class Balance::ReverseCalculator < Balance::BaseCalculator
         if valuation.present?
           @balances << build_balance(date, previous_cash_balance, holdings_value)
         else
-          @balances << build_balance(date, current_cash_balance, holdings_value)
+          # If date is today, we don't distinguish cash vs. total since provider's are inconsistent with treatment
+          # of the cash component.  Instead, just set the balance equal to the "total value" reported by the provider
+          if date == Date.current
+            @balances << build_balance(date, account.cash_balance, account.balance - account.cash_balance)
+          else
+            @balances << build_balance(date, current_cash_balance, holdings_value)
+          end
         end
 
         current_cash_balance = previous_cash_balance

app/models/balance/trend_calculator.rb

@@ -5,90 +5,26 @@
 class Balance::TrendCalculator
   BalanceTrend = Struct.new(:trend, :cash, keyword_init: true)
 
-  class << self
-    def for(entries)
-      return nil if entries.blank?
-
-      account = entries.first.account
-
-      date_range = entries.minmax_by(&:date)
-      min_entry_date, max_entry_date = date_range.map(&:date)
-
-      # In case view is filtered and there are entry gaps, refetch all entries in range
-      all_entries = account.entries.where(date: min_entry_date..max_entry_date).chronological.to_a
-      balances = account.balances.where(date: (min_entry_date - 1.day)..max_entry_date).chronological.to_a
-      holdings = account.holdings.where(date: (min_entry_date - 1.day)..max_entry_date).to_a
-
-      new(all_entries, balances, holdings)
-    end
-  end
-
-  def initialize(entries, balances, holdings)
-    @entries = entries
+  def initialize(balances)
     @balances = balances
-    @holdings = holdings
   end
 
-  def trend_for(entry)
-    intraday_balance = nil
-    intraday_cash_balance = nil
+  def trend_for(date)
+    balance = @balances.find { |b| b.date == date }
+    prior_balance = @balances.find { |b| b.date == date - 1.day }
 
-    start_of_day_balance = balances.find { |b| b.date == entry.date - 1.day && b.currency == entry.currency }
-    end_of_day_balance = balances.find { |b| b.date == entry.date && b.currency == entry.currency }
-
-    return BalanceTrend.new(trend: nil) if start_of_day_balance.blank? || end_of_day_balance.blank?
-
-    todays_holdings_value = holdings.select { |h| h.date == entry.date }.sum(&:amount)
-
-    prior_balance = start_of_day_balance.balance
-    prior_cash_balance = start_of_day_balance.cash_balance
-    current_balance = nil
-    current_cash_balance = nil
-
-    todays_entries = entries.select { |e| e.date == entry.date }
-
-    todays_entries.each_with_index do |e, idx|
-      if e.valuation?
-        current_balance = e.amount
-        current_cash_balance = e.amount
-      else
-        multiplier = e.account.liability? ? 1 : -1
-        balance_change = e.trade? ? 0 : multiplier * e.amount
-        cash_change = multiplier * e.amount
-
-        current_balance = prior_balance + balance_change
-        current_cash_balance = prior_cash_balance + cash_change
-      end
-
-      if e.id == entry.id
-        # Final entry should always match the end-of-day balances
-        if idx == todays_entries.size - 1
-          intraday_balance = end_of_day_balance.balance
-          intraday_cash_balance = end_of_day_balance.cash_balance
-        else
-          intraday_balance = current_balance
-          intraday_cash_balance = current_cash_balance
-        end
-
-        break
-      else
-        prior_balance = current_balance
-        prior_cash_balance = current_cash_balance
-      end
-    end
-
-    return BalanceTrend.new(trend: nil) unless intraday_balance.present?
+    return BalanceTrend.new(trend: nil) unless balance.present?
 
     BalanceTrend.new(
       trend: Trend.new(
-        current: Money.new(intraday_balance, entry.currency),
-        previous: Money.new(prior_balance, entry.currency),
-        favorable_direction: entry.account.favorable_direction
+        current: Money.new(balance.balance, balance.currency),
+        previous: Money.new(prior_balance.balance, balance.currency),
+        favorable_direction: balance.account.favorable_direction
       ),
-      cash: Money.new(intraday_cash_balance, entry.currency),
+      cash: Money.new(balance.cash_balance, balance.currency),
     )
   end
 
   private
-    attr_reader :entries, :balances, :holdings
+    attr_reader :balances
 end

app/models/plaid_investment_sync.rb

@@ -11,6 +11,7 @@ class PlaidInvestmentSync
     @securities = securities
 
     PlaidAccount.transaction do
+      normalize_cash_balance!
       sync_transactions!
       sync_holdings!
     end
@@ -19,6 +20,23 @@ class PlaidInvestmentSync
   private
     attr_reader :transactions, :holdings, :securities
 
+    # Plaid considers "brokerage cash" and "cash equivalent holdings" to all be part of "cash balance"
+    # Internally, we DO NOT.
+    # Maybe clearly distinguishes between "brokerage cash" vs. "holdings (i.e. invested cash)"
+    # For this reason, we must back out cash + cash equivalent holdings from the reported cash balance to avoid double counting
+    def normalize_cash_balance!
+      excludable_cash_holdings = holdings.select do |h|
+        internal_security, plaid_security = get_security(h.security_id, securities)
+        internal_security.present? && (plaid_security&.is_cash_equivalent || plaid_security&.type == "cash")
+      end
+
+      excludable_cash_holdings_value = excludable_cash_holdings.sum { |h| h.quantity * h.institution_price }
+
+      plaid_account.account.update!(
+        cash_balance: plaid_account.account.cash_balance - excludable_cash_holdings_value
+      )
+    end
+
     def sync_transactions!
       transactions.each do |transaction|
         security, plaid_security = get_security(transaction.security_id, securities)

app/views/accounts/show/_activity.html.erb

@@ -77,10 +77,11 @@
         <div>
           <div class="rounded-tl-lg rounded-tr-lg bg-container border-alpha-black-25 shadow-xs">
             <div class="space-y-4">
-              <% calculator = Balance::TrendCalculator.for(@entries) %>
+              <% calculator = Balance::TrendCalculator.new(@account.balances) %>
+
               <%= entries_by_date(@entries) do |entries| %>
-                <% entries.each do |entry| %>
-                  <%= render entry, balance_trend: calculator&.trend_for(entry), view_ctx: "account" %>
+                <% entries.each_with_index do |entry, index| %>
+                  <%= render entry, balance_trend: index == 0 ? calculator.trend_for(entry.date) : nil, view_ctx: "account" %>
                 <% end %>
               <% end %>
             </div>

app/views/accounts/show/_chart.html.erb

@@ -28,7 +28,7 @@
                     Period.as_options,
                     { selected: period.key },
                     data: { "auto-submit-form-target": "auto" },
-                    class: "bg-container border border-secondary font-medium rounded-lg px-3 py-2 text-sm pr-7 cursor-pointer text-primary focus:outline-hidden focus:ring-0" %>
+                    class: "bg-container border border-secondary rounded-lg px-3 py-2 text-sm pr-7 cursor-pointer text-primary focus:outline-hidden focus:ring-0" %>
       </div>
     <% end %>
   </div>

app/views/transactions/_transaction.html.erb

@@ -4,12 +4,11 @@
 
 <%= turbo_frame_tag dom_id(entry) do %>
   <%= turbo_frame_tag dom_id(transaction) do %>
-    <div class="grid grid-cols-12 items-center text-primary text-sm font-medium p-4 md:p-4
+    <div class="grid grid-cols-12 items-center text-primary text-sm font-medium p-4 lg:p-4
                 <%= @focused_record == entry || @focused_record == transaction ?
                     "border border-gray-900 rounded-lg" : "" %>">
 
-      <div class="pr-4 md:pr-10 flex items-center gap-3 md:gap-4
-                  <%= balance_trend ? "col-span-8 md:col-span-6" : "col-span-8" %>">
+      <div class="pr-4 lg:pr-10 flex items-center gap-3 lg:gap-4 col-span-8 lg:col-span-6">
         <%= check_box_tag dom_id(entry, "selection"),
             disabled: transaction.transfer?,
             class: "checkbox checkbox--light",
@@ -58,7 +57,7 @@
                   <% end %>
                 </div>
 
-                <div class="text-secondary text-xs font-normal hidden md:block">
+                <div class="text-secondary text-xs font-normal hidden lg:block">
                   <% if transaction.transfer? %>
                     <%= render "transfers/account_links",
                         transfer: transaction.transfer,
@@ -76,26 +75,24 @@
         </div>
       </div>
 
-      <div class="hidden md:flex items-center gap-1 col-span-2">
+      <div class="hidden lg:flex items-center gap-1 col-span-2">
         <%= render "transactions/transaction_category", transaction: transaction %>
       </div>
 
-      <div class="col-span-4 md:col-span-2 ml-auto text-right">
+      <div class="col-span-4 lg:col-span-2 ml-auto text-right">
         <%= content_tag :p,
             transaction.transfer? && view_ctx == "global" ? "+/- #{format_money(entry.amount_money.abs)}" : format_money(-entry.amount_money),
             class: ["text-green-600": entry.amount.negative?] %>
       </div>
 
-      <% if balance_trend %>
-        <div class="col-span-2 justify-self-end hidden md:block">
-          <% if balance_trend.trend %>
-            <%= tag.p format_money(balance_trend.trend.current),
+      <div class="col-span-2 justify-self-end hidden lg:block">
+        <% if balance_trend&.trend %>
+          <%= tag.p format_money(balance_trend.trend.current),
                 class: "font-medium text-sm text-primary" %>
-          <% else %>
-            <%= tag.p "--", class: "font-medium text-sm text-gray-400" %>
-          <% end %>
-        </div>
-      <% end %>
+        <% else %>
+          <%= tag.p "--", class: "font-medium text-sm text-gray-400" %>
+        <% end %>
+      </div>
     </div>
   <% end %>
 <% end %>

app/views/valuations/_valuation.html.erb

@@ -14,9 +14,7 @@
                         data: { id: entry.id, "bulk-select-target": "row", action: "bulk-select#toggleRowSelection" } %>
 
         <div class="flex items-center gap-3">
-          <%= tag.div class: "w-6 h-6 rounded-full p-1.5 flex items-center justify-center", style: "color: #{color}" do %>
-            <%= icon(icon, size: "sm", color: "current") %>
-          <% end %>
+          <%= render FilledIconComponent.new(icon: icon, size: "sm", hex_color: color, rounded: true) %>
 
           <div class="truncate text-primary">
             <%= link_to entry.name,

test/models/balance/reverse_calculator_test.rb

@@ -120,4 +120,23 @@ class Balance::ReverseCalculatorTest < ActiveSupport::TestCase
 
     assert_equal expected, calculated
   end
+
+  test "uses provider reported holdings and cash value on current day" do
+    aapl = securities(:aapl)
+
+    # Implied holdings value of $1,000 from provider
+    @account.update!(cash_balance: 19000, balance: 20000)
+
+    # Create a holding that differs in value from provider ($2,000 vs. the $1,000 reported by provider)
+    Holding.create!(date: Date.current, account: @account, security: aapl, qty: 10, price: 100, amount: 2000, currency: "USD")
+    Holding.create!(date: 1.day.ago.to_date, account: @account, security: aapl, qty: 10, price: 100, amount: 2000, currency: "USD")
+
+    # Today reports the provider value.  Yesterday, provider won't give us any data, so we MUST look at the generated holdings value
+    # to calculate the end balance ($19,000 cash + $2,000 holdings = $21,000 total value)
+    expected = [ 21000, 20000 ]
+
+    calculated = Balance::ReverseCalculator.new(@account).calculate.sort_by(&:date).map(&:balance)
+
+    assert_equal expected, calculated
+  end
 end