Preserve original transaction names when enriching (#1556)

* Preserve original transaction name

* Remove stale method

* Fix tests

app/controllers/concerns/entryable_resource.rb

@@ -119,7 +119,7 @@ module EntryableResource
 
     def entry_params
       params.require(:account_entry).permit(
-        :account_id, :name, :date, :amount, :currency, :excluded, :notes, :nature,
+        :account_id, :name, :enriched_name, :date, :amount, :currency, :excluded, :notes, :nature,
         entryable_attributes: self.class.permitted_entryable_attributes
       )
     end

app/models/account.rb

@@ -151,6 +151,7 @@ class Account < ApplicationRecord
     else
       entries.create! \
         date: Date.current,
+        name: "Balance update",
         amount: balance,
         currency: currency,
         entryable: Account::Valuation.new

app/models/account/data_enricher.rb

@@ -50,7 +50,7 @@ class Account::DataEnricher
               category.save! if category.present?
               entry.update!(
                 enriched_at: Time.current,
-                name: entry.enriched_at.nil? && info.name ? info.name : entry.name,
+                enriched_name: info.name,
                 entryable_attributes: entryable_attributes
               )
             end

app/models/account/entry.rb

@@ -10,7 +10,7 @@ class Account::Entry < ApplicationRecord
   delegated_type :entryable, types: Account::Entryable::TYPES, dependent: :destroy
   accepts_nested_attributes_for :entryable
 
-  validates :date, :amount, :currency, presence: true
+  validates :date, :name, :amount, :currency, presence: true
   validates :date, uniqueness: { scope: [ :account_id, :entryable_type ] }, if: -> { account_valuation? }
   validates :date, comparison: { greater_than: -> { min_supported_date } }
 
@@ -47,14 +47,6 @@ class Account::Entry < ApplicationRecord
     account.sync_later(start_date: sync_start_date)
   end
 
-  def inflow?
-    amount <= 0 && account_transaction?
-  end
-
-  def outflow?
-    amount > 0 && account_transaction?
-  end
-
   def entryable_name_short
     entryable_type.demodulize.underscore
   end
@@ -63,6 +55,10 @@ class Account::Entry < ApplicationRecord
     Account::BalanceTrendCalculator.new(self, entries, balances).trend
   end
 
+  def display_name
+    enriched_name.presence || name
+  end
+
   class << self
     # arbitrary cutoff date to avoid expensive sync operations
     def min_supported_date

app/models/account/syncer.rb

@@ -11,7 +11,8 @@ class Account::Syncer
     update_account_info(balances, holdings) unless account.plaid_account_id.present?
     convert_records_to_family_currency(balances, holdings) unless account.currency == account.family.currency
 
-    if account.family.data_enrichment_enabled?
+    # Enrich if user opted in or if we're syncing transactions from a Plaid account
+    if account.family.data_enrichment_enabled? || account.plaid_account_id.present?
       account.enrich_data_later
     else
       Rails.logger.info("Data enrichment is disabled, skipping enrichment for account #{account.id}")

app/models/account/trade.rb

@@ -26,11 +26,6 @@ class Account::Trade < ApplicationRecord
     qty > 0
   end
 
-  def name
-    prefix = sell? ? "Sell " : "Buy "
-    prefix + "#{qty.abs} shares of #{security.ticker}"
-  end
-
   def unrealized_gain_loss
     return nil if sell?
     current_price = security.current_price

app/models/account/trade_builder.rb

@@ -31,7 +31,11 @@ class Account::TradeBuilder
     end
 
     def build_trade
+      prefix = type == "sell" ? "Sell " : "Buy "
+      trade_name = prefix + "#{qty.to_i.abs} shares of #{security.ticker}"
+
       account.entries.new(
+        name: trade_name,
         date: date,
         amount: signed_amount,
         currency: currency,

app/models/account/transaction.rb

@@ -48,10 +48,6 @@ class Account::Transaction < ApplicationRecord
       end
   end
 
-  def name
-    entry.name || (entry.amount.positive? ? "Expense" : "Income")
-  end
-
   def eod_balance
     entry.amount_money
   end

app/models/account/transfer.rb

@@ -33,11 +33,11 @@ class Account::Transfer < ApplicationRecord
   end
 
   def inflow_transaction
-    entries.find { |e| e.inflow? }
+    entries.find { |e| e.amount.negative? }
   end
 
   def outflow_transaction
-    entries.find { |e| e.outflow? }
+    entries.find { |e| e.amount.positive? }
   end
 
   def update_entries!(params)

app/models/account/valuation.rb

@@ -10,8 +10,4 @@ class Account::Valuation < ApplicationRecord
       false
     end
   end
-
-  def name
-    "Balance update"
-  end
 end

app/models/demo/generator.rb

@@ -303,6 +303,7 @@ class Demo::Generator
         date: date,
         amount: amount,
         currency: "USD",
+        name: "Balance update",
         entryable: Account::Valuation.new
     end
 

app/views/account/trades/_trade.html.erb

@@ -13,14 +13,14 @@
     <div class="max-w-full">
       <%= tag.div class: ["flex items-center gap-2"] do %>
         <div class="flex h-8 w-8 shrink-0 items-center justify-center rounded-full bg-gray-600/5 text-gray-600">
-          <%= trade.name.first.upcase %>
+          <%= entry.display_name.first.upcase %>
         </div>
 
         <div class="truncate">
           <% if entry.new_record? %>
-            <%= content_tag :p, trade.name %>
+            <%= content_tag :p, entry.display_name %>
           <% else %>
-            <%= link_to trade.name,
+            <%= link_to entry.display_name,
                         account_entry_path(entry),
                         data: { turbo_frame: "drawer", turbo_prefetch: false },
                         class: "hover:underline hover:text-gray-800" %>

app/views/account/transactions/_transaction.html.erb

@@ -11,17 +11,17 @@
 
     <div class="max-w-full">
       <%= content_tag :div, class: ["flex items-center gap-2"] do %>
-        <% if entry.account_transaction.merchant&.icon_url %>
+        <% if transaction.merchant&.icon_url %>
-          <%= image_tag entry.account_transaction.merchant.icon_url, class: "w-6 h-6 rounded-full" %>
+          <%= image_tag transaction.merchant.icon_url, class: "w-6 h-6 rounded-full" %>
         <% else %>
-          <%= render "shared/circle_logo", name: transaction.name, size: "sm" %>
+          <%= render "shared/circle_logo", name: entry.display_name, size: "sm" %>
         <% end %>
 
         <div class="truncate">
           <% if entry.new_record? %>
-            <%= content_tag :p, transaction.name %>
+            <%= content_tag :p, entry.display_name %>
           <% else %>
-            <%= link_to transaction.name,
+            <%= link_to entry.display_name,
                         entry.transfer.present? ? account_transfer_path(entry.transfer) : account_entry_path(entry),
                         data: { turbo_frame: "drawer", turbo_prefetch: false },
                         class: "hover:underline hover:text-gray-800" %>
@@ -41,7 +41,7 @@
     <% end %>
 
     <div class="col-span-2">
-      <%= render "account/transfers/account_logos", transfer: entry.transfer, outflow: entry.outflow? %>
+      <%= render "account/transfers/account_logos", transfer: entry.transfer, outflow: entry.amount.positive? %>
     </div>
   <% else %>
     <div class="flex items-center gap-1 col-span-2">
@@ -65,7 +65,7 @@
   <div class="col-span-2 ml-auto">
     <%= content_tag :p,
                     format_money(-entry.amount_money),
-                    class: ["text-green-600": entry.inflow?] %>
+                    class: ["text-green-600": entry.amount.negative?] %>
   </div>
 
   <% if balance_trend %>

app/views/account/transactions/show.html.erb

@@ -9,7 +9,8 @@
               url: account_transaction_path(@entry),
               class: "space-y-2",
               data: { controller: "auto-submit-form" } do |f| %>
-          <%= f.text_field :name,
+
+          <%= f.text_field @entry.enriched_at.present? ? :enriched_name : :name,
                 label: t(".name_label"),
                 "data-auto-submit-form-target": "auto" %>
 

app/views/account/valuations/_form.html.erb

@@ -8,6 +8,7 @@
   <% end %>
 
   <div class="space-y-3">
+    <%= form.hidden_field :name, value: "Balance update" %>
     <%= form.date_field :date, label: true, required: true, value: Date.current, min: Account::Entry.min_supported_date, max: Date.current %>
     <%= form.money_field :amount, label: t(".amount"), required: true %>
   </div>

app/views/account/valuations/_valuation.html.erb

@@ -18,9 +18,9 @@
 
       <div class="truncate text-gray-900">
         <% if entry.new_record? %>
-          <%= content_tag :p, entry.entryable.name %>
+          <%= content_tag :p, entry.display_name %>
         <% else %>
-          <%= link_to entry.entryable.name,
+          <%= link_to entry.display_name,
                         account_entry_path(entry),
                         data: { turbo_frame: "drawer", turbo_prefetch: false },
                         class: "hover:underline hover:text-gray-800" %>

config/brakeman.ignore

@@ -23,6 +23,29 @@
       ],
       "note": ""
     },
+    {
+      "warning_type": "Mass Assignment",
+      "warning_code": 105,
+      "fingerprint": "5bfdb129316655dc4e02f3a599156660414a6562212a5f61057d376f6888f078",
+      "check_name": "PermitAttributes",
+      "message": "Potentially dangerous key allowed for mass assignment",
+      "file": "app/controllers/concerns/entryable_resource.rb",
+      "line": 122,
+      "link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/",
+      "code": "params.require(:account_entry).permit(:account_id, :name, :enriched_name, :date, :amount, :currency, :excluded, :notes, :nature, :entryable_attributes => self.class.permitted_entryable_attributes)",
+      "render_path": null,
+      "location": {
+        "type": "method",
+        "class": "EntryableResource",
+        "method": "entry_params"
+      },
+      "user_input": ":account_id",
+      "confidence": "High",
+      "cwe_id": [
+        915
+      ],
+      "note": ""
+    },
     {
       "warning_type": "Mass Assignment",
       "warning_code": 105,
@@ -80,29 +103,6 @@
       ],
       "note": ""
     },
-    {
-      "warning_type": "Mass Assignment",
-      "warning_code": 105,
-      "fingerprint": "f158202dcc66f2273ddea5e5296bad7146a50ca6667f49c77372b5b234542334",
-      "check_name": "PermitAttributes",
-      "message": "Potentially dangerous key allowed for mass assignment",
-      "file": "app/controllers/concerns/entryable_resource.rb",
-      "line": 122,
-      "link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/",
-      "code": "params.require(:account_entry).permit(:account_id, :name, :date, :amount, :currency, :excluded, :notes, :nature, :entryable_attributes => self.class.permitted_entryable_attributes)",
-      "render_path": null,
-      "location": {
-        "type": "method",
-        "class": "EntryableResource",
-        "method": "entry_params"
-      },
-      "user_input": ":account_id",
-      "confidence": "High",
-      "cwe_id": [
-        915
-      ],
-      "note": ""
-    },
     {
       "warning_type": "Dynamic Render Path",
       "warning_code": 15,
@@ -138,6 +138,6 @@
       "note": ""
     }
   ],
-  "updated": "2024-11-27 15:33:53 -0500",
+  "updated": "2024-12-18 17:46:13 -0500",
   "brakeman_version": "6.2.2"
 }

db/migrate/20241218132503_add_enriched_name_field.rb

@@ -0,0 +1,37 @@
+class AddEnrichedNameField < ActiveRecord::Migration[7.2]
+  def change
+    add_column :account_entries, :enriched_name, :string
+
+    reversible do |dir|
+      dir.up do
+        execute <<-SQL
+          UPDATE account_entries ae
+          SET name = CASE ae.entryable_type
+            WHEN 'Account::Trade' THEN
+              CASE
+                WHEN EXISTS (
+                  SELECT 1 FROM account_trades t
+                  WHERE t.id = ae.entryable_id AND t.qty < 0
+                ) THEN 'Sell trade'
+                ELSE 'Buy trade'
+              END
+            WHEN 'Account::Transaction' THEN
+              CASE
+                WHEN ae.amount > 0 THEN 'Expense'
+                ELSE 'Income'
+              END
+            WHEN 'Account::Valuation' THEN 'Balance update'
+            ELSE 'Unknown entry'
+          END
+          WHERE name IS NULL
+        SQL
+
+        change_column_null :account_entries, :name, false
+      end
+
+      dir.down do
+        change_column_null :account_entries, :name, true
+      end
+    end
+  end
+end

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: 2024_12_17_141716) do
+ActiveRecord::Schema[7.2].define(version: 2024_12_18_132503) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pgcrypto"
   enable_extension "plpgsql"
@@ -39,7 +39,7 @@ ActiveRecord::Schema[7.2].define(version: 2024_12_17_141716) do
     t.decimal "amount", precision: 19, scale: 4
     t.string "currency"
     t.date "date"
-    t.string "name"
+    t.string "name", null: false
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.uuid "transfer_id"
@@ -49,6 +49,7 @@ ActiveRecord::Schema[7.2].define(version: 2024_12_17_141716) do
     t.boolean "excluded", default: false
     t.string "plaid_id"
     t.datetime "enriched_at"
+    t.string "enriched_name"
     t.index ["account_id"], name: "index_account_entries_on_account_id"
     t.index ["import_id"], name: "index_account_entries_on_import_id"
     t.index ["transfer_id"], name: "index_account_entries_on_transfer_id"

test/interfaces/accountable_resource_interface_test.rb

@@ -76,7 +76,7 @@ module AccountableResourceInterfaceTest
   end
 
   test "updates account balance by editing existing valuation for today" do
-    @account.entries.create! date: Date.current, amount: 6000, currency: "USD", entryable: Account::Valuation.new
+    @account.entries.create! date: Date.current, amount: 6000, currency: "USD", name: "Balance update", entryable: Account::Valuation.new
 
     assert_no_difference [ "Account::Entry.count", "Account::Valuation.count" ] do
       patch account_url(@account), params: {

test/models/account/entry_test.rb

@@ -90,10 +90,4 @@ class Account::EntryTest < ActiveSupport::TestCase
 
     assert_equal Money.new(-200), family.entries.income_total("USD")
   end
-
-  # See: https://github.com/maybe-finance/maybe/wiki/vision#signage-of-money
-  test "transactions with negative amounts are inflows, positive amounts are outflows to an account" do
-    assert create_transaction(amount: -10).inflow?
-    assert create_transaction(amount: 10).outflow?
-  end
 end

test/system/trades_test.rb

@@ -23,7 +23,7 @@ class TradesTest < ApplicationSystemTestCase
   end
 
   test "can create buy transaction" do
-    shares_qty = 25.0
+    shares_qty = 25
 
     open_new_trade_modal
 
@@ -38,7 +38,7 @@ class TradesTest < ApplicationSystemTestCase
     visit_account_trades
 
     within_trades do
-      assert_text "Buy 10.0 shares of AAPL"
+      assert_text "Purchase 10 shares of AAPL"
       assert_text "Buy #{shares_qty} shares of AAPL"
     end
   end
@@ -60,7 +60,7 @@ class TradesTest < ApplicationSystemTestCase
     visit_account_trades
 
     within_trades do
-      assert_text "Sell #{aapl.qty} shares of AAPL"
+      assert_text "Sell #{aapl.qty.round} shares of AAPL"
     end
   end