Fix Account Holding validation and synchronization (#1818)

* Fix Account Holding validation and synchronization

Fixes #1781

- Add comprehensive validations for Account::Holding
- Implement validation to ensure amount matches qty * price
- Update Account::Syncer to include qty and price during synchronization
- Add database constraints for holding attributes and calculations

* Remove database check constraints for Account Holdings

Align with project convention of keeping complex validations in ActiveRecord
- Remove database-level check constraints for quantity, price, and amount
- Maintain database-level null and unique constraints
- Prepare for more flexible validation in the model layer

.cursor/rules/project-conventions.mdc

@@ -76,3 +76,8 @@ Due to the open-source nature of this project, we have chosen Minitest + Fixture
 - Keep fixtures to a minimum.  Most models should have 2-3 fixtures maximum that represent the "base cases" for that model.  "Edge cases" should be created on the fly, within the context of the test which it is needed.
 - For tests that require a large number of fixture records to be created, use Rails helpers such as [entries_test_helper.rb](mdc:test/support/account/entries_test_helper.rb) to act as a "factory" for creating these.  For a great example of this, check out [balance_calculator_test.rb](mdc:test/models/account/balance_calculator_test.rb)
 
+### Convention 7: Use ActiveRecord for complex validations, DB for simple ones, keep business logic out of DB
+
+- Enforce `null` checks, unique indexes, and other simple validations in the DB
+- ActiveRecord validations _may_ mirror the DB level ones, but not 100% necessary.  These are for convenience when error handling in forms.  Always prefer client-side form validation when possible.
+- Complex validations and business logic should remain in ActiveRecord

app/models/account/holding.rb

@@ -6,7 +6,9 @@ class Account::Holding < ApplicationRecord
   belongs_to :account
   belongs_to :security
 
-  validates :qty, :currency, presence: true
+  validates :qty, :currency, :date, :price, :amount, presence: true
+  validates :qty, :price, :amount, numericality: { greater_than_or_equal_to: 0 }
+  validate :amount_matches_qty_and_price
 
   scope :chronological, -> { order(:date) }
   scope :for, ->(security) { where(security_id: security).order(:date) }
@@ -62,4 +64,13 @@ class Account::Holding < ApplicationRecord
         current: amount_money,
         previous: start_amount
     end
+
+    def amount_matches_qty_and_price
+      return if qty.blank? || price.blank? || amount.blank?
+
+      expected_amount = qty * price
+      return if amount == expected_amount
+
+      errors.add(:amount, "must equal qty * price (expected: #{expected_amount})")
+    end
 end

app/models/account/syncer.rb

@@ -99,6 +99,8 @@ class Account::Syncer
         account.holdings.build(
           security: holding.security,
           date: holding.date,
+          qty: holding.qty,
+          price: exchange_rate.rate * holding.price,
           amount: exchange_rate.rate * holding.amount,
           currency: to_currency
         )

db/migrate/20250206204404_add_constraints_to_account_holdings.rb

@@ -0,0 +1,40 @@
+class AddConstraintsToAccountHoldings < ActiveRecord::Migration[7.2]
+  def up
+    # First, remove any holdings with nil values
+    execute <<-SQL
+      DELETE FROM account_holdings#{' '}
+      WHERE date IS NULL#{' '}
+         OR qty IS NULL#{' '}
+         OR price IS NULL#{' '}
+         OR amount IS NULL#{' '}
+         OR currency IS NULL;
+    SQL
+
+    # Remove any holdings where amount doesn't match qty * price
+    execute <<-SQL
+      DELETE FROM account_holdings#{' '}
+      WHERE ROUND(qty * price, 4) != ROUND(amount, 4);
+    SQL
+
+    # Remove any holdings with negative values
+    execute <<-SQL
+      DELETE FROM account_holdings#{' '}
+      WHERE qty < 0 OR price < 0 OR amount < 0;
+    SQL
+
+    # Now add NOT NULL constraints
+    change_column_null :account_holdings, :date, false
+    change_column_null :account_holdings, :qty, false
+    change_column_null :account_holdings, :price, false
+    change_column_null :account_holdings, :amount, false
+    change_column_null :account_holdings, :currency, false
+  end
+
+  def down
+    change_column_null :account_holdings, :date, true
+    change_column_null :account_holdings, :qty, true
+    change_column_null :account_holdings, :price, true
+    change_column_null :account_holdings, :amount, true
+    change_column_null :account_holdings, :currency, true
+  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: 2025_02_06_151825) do
+ActiveRecord::Schema[7.2].define(version: 2025_02_06_204404) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pgcrypto"
   enable_extension "plpgsql"
@@ -55,11 +55,11 @@ ActiveRecord::Schema[7.2].define(version: 2025_02_06_151825) do
   create_table "account_holdings", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
     t.uuid "account_id", null: false
     t.uuid "security_id", null: false
-    t.date "date"
-    t.decimal "qty", precision: 19, scale: 4
-    t.decimal "price", precision: 19, scale: 4
-    t.decimal "amount", precision: 19, scale: 4
-    t.string "currency"
+    t.date "date", null: false
+    t.decimal "qty", precision: 19, scale: 4, null: false
+    t.decimal "price", precision: 19, scale: 4, null: false
+    t.decimal "amount", precision: 19, scale: 4, null: false
+    t.string "currency", null: false
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.index ["account_id", "security_id", "date", "currency"], name: "idx_on_account_id_security_id_date_currency_234024c8e3", unique: true

test/models/account/syncer_test.rb

@@ -29,8 +29,8 @@ class Account::SyncerTest < ActiveSupport::TestCase
 
     Account::HoldingCalculator.any_instance.expects(:calculate).returns(
       [
-        Account::Holding.new(security: securities(:aapl), date: 1.day.ago.to_date, amount: 500, currency: "EUR"),
-        Account::Holding.new(security: securities(:aapl), date: Date.current, amount: 500, currency: "EUR")
+        Account::Holding.new(security: securities(:aapl), date: 1.day.ago.to_date, qty: 10, price: 50, amount: 500, currency: "EUR"),
+        Account::Holding.new(security: securities(:aapl), date: Date.current, qty: 10, price: 50, amount: 500, currency: "EUR")
       ]
     )