diff --git a/.cursor/rules/project-conventions.mdc b/.cursor/rules/project-conventions.mdc index 46b1032d..eb139262 100644 --- a/.cursor/rules/project-conventions.mdc +++ b/.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 \ No newline at end of file diff --git a/app/models/account/holding.rb b/app/models/account/holding.rb index bd2e1ca3..7f0bad7a 100644 --- a/app/models/account/holding.rb +++ b/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 diff --git a/app/models/account/syncer.rb b/app/models/account/syncer.rb index 867c9052..e8bf7a40 100644 --- a/app/models/account/syncer.rb +++ b/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 ) diff --git a/db/migrate/20250206204404_add_constraints_to_account_holdings.rb b/db/migrate/20250206204404_add_constraints_to_account_holdings.rb new file mode 100644 index 00000000..d745346d --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 056caf8c..a7ec28bc 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: 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 diff --git a/test/models/account/syncer_test.rb b/test/models/account/syncer_test.rb index bfbf4a87..261ab28c 100644 --- a/test/models/account/syncer_test.rb +++ b/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") ] )