From 15f8d827b5453ac91135961372798b0abfa21402 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Mon, 7 Jul 2025 11:31:37 -0400 Subject: [PATCH 01/12] Account balance anchors --- app/models/account.rb | 32 ++++------ app/models/account/balance_updater.rb | 6 +- app/models/entry.rb | 40 ++++++++++++ app/models/valuation.rb | 26 ++++++++ app/models/valuation/name.rb | 63 +++++++++++++++++++ ...34_add_valuation_kind_field_for_anchors.rb | 31 +++++++++ db/schema.rb | 6 +- lib/tasks/data_migration.rake | 43 +++++++++++++ 8 files changed, 226 insertions(+), 21 deletions(-) create mode 100644 app/models/valuation/name.rb create mode 100644 db/migrate/20250707130134_add_valuation_kind_field_for_anchors.rb diff --git a/app/models/account.rb b/app/models/account.rb index 43e02cfd..09800168 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -59,28 +59,22 @@ class Account < ApplicationRecord def create_and_sync(attributes) attributes[:accountable_attributes] ||= {} # Ensure accountable is created, even if empty account = new(attributes.merge(cash_balance: attributes[:balance])) - initial_balance = attributes.dig(:accountable_attributes, :initial_balance)&.to_d || 0 + initial_balance = attributes.dig(:accountable_attributes, :initial_balance)&.to_d || account.balance - transaction do - # Create 2 valuations for new accounts to establish a value history for users to see - account.entries.build( - name: "Current Balance", - date: Date.current, - amount: account.balance, - currency: account.currency, - entryable: Valuation.new - ) - account.entries.build( - name: "Initial Balance", - date: 1.day.ago.to_date, - amount: initial_balance, - currency: account.currency, - entryable: Valuation.new + account.entries.build( + name: Valuation::Name.new("opening_anchor", account.accountable_type).to_s, + date: 2.years.ago.to_date, + amount: initial_balance, + currency: account.currency, + entryable: Valuation.new( + kind: "opening_anchor", + balance: initial_balance, + cash_balance: initial_balance, + currency: account.currency ) + ) - account.save! - end - + account.save! account.sync_later account end diff --git a/app/models/account/balance_updater.rb b/app/models/account/balance_updater.rb index d006df3f..1b50a9b5 100644 --- a/app/models/account/balance_updater.rb +++ b/app/models/account/balance_updater.rb @@ -23,7 +23,7 @@ class Account::BalanceUpdater valuation_entry.amount = balance valuation_entry.currency = currency if currency.present? - valuation_entry.name = "Manual #{account.accountable.balance_display_name} update" + valuation_entry.name = valuation_name(valuation_entry.entryable, account) valuation_entry.notes = notes if notes.present? valuation_entry.save! end @@ -44,4 +44,8 @@ class Account::BalanceUpdater def requires_update? date != Date.current || account.balance != balance || account.currency != currency end + + def valuation_name(valuation_entry, account) + Valuation::Name.new(valuation_entry.entryable.kind, account.accountable_type).to_s + end end diff --git a/app/models/entry.rb b/app/models/entry.rb index 332cbee9..f59810b6 100644 --- a/app/models/entry.rb +++ b/app/models/entry.rb @@ -14,6 +14,11 @@ class Entry < ApplicationRecord validates :date, uniqueness: { scope: [ :account_id, :entryable_type ] }, if: -> { valuation? } validates :date, comparison: { greater_than: -> { min_supported_date } } + # To ensure we can recreate balance history solely from Entries, all entries must post on or before the current anchor (i.e. "Current balance"), + # and after the opening anchor (i.e. "Opening balance"). This domain invariant should be enforced by the Account model when adding/modifying entries. + validate :date_after_opening_anchor + validate :date_on_or_before_current_anchor + scope :visible, -> { joins(:account).where(accounts: { status: [ "draft", "active" ] }) } @@ -96,4 +101,39 @@ class Entry < ApplicationRecord all.size end end + + private + def date_after_opening_anchor + return unless account && date + + # Skip validation for anchor valuations themselves + return if valuation? && entryable.kind.in?(%w[opening_anchor current_anchor]) + + opening_anchor_date = account.valuations + .joins(:entry) + .where(kind: "opening_anchor") + .pluck(Arel.sql("entries.date")) + .first + + if opening_anchor_date && date <= opening_anchor_date + errors.add(:date, "must be after the opening balance date (#{opening_anchor_date})") + end + end + + def date_on_or_before_current_anchor + return unless account && date + + # Skip validation for anchor valuations themselves + return if valuation? && entryable.kind.in?(%w[opening_anchor current_anchor]) + + current_anchor_date = account.valuations + .joins(:entry) + .where(kind: "current_anchor") + .pluck(Arel.sql("entries.date")) + .first + + if current_anchor_date && date > current_anchor_date + errors.add(:date, "must be on or before the current balance date (#{current_anchor_date})") + end + end end diff --git a/app/models/valuation.rb b/app/models/valuation.rb index 6d1d2b4b..b3a823cf 100644 --- a/app/models/valuation.rb +++ b/app/models/valuation.rb @@ -1,3 +1,29 @@ class Valuation < ApplicationRecord include Entryable + + enum :kind, { + recon: "recon", # A balance reconciliation that sets the Account balance from this point forward (often defined by user) + snapshot: "snapshot", # An "event-sourcing snapshot", which is purely for performance so less history is required to derive the balance + opening_anchor: "opening_anchor", # Each account has a single opening anchor, which defines the opening balance on the account + current_anchor: "current_anchor" # Each account has a single current anchor, which defines the current balance on the account + }, validate: true + + # Each account can have at most 1 opening anchor and 1 current anchor. All valuations between these anchors should + # be either "recon" or "snapshot". This ensures we can reliably construct the account balance history solely from Entries. + validate :unique_anchor_per_account, if: -> { opening_anchor? || current_anchor? } + + private + def unique_anchor_per_account + return unless entry&.account + + existing_anchor = entry.account.valuations + .joins(:entry) + .where(kind: kind) + .where.not(id: id) + .exists? + + if existing_anchor + errors.add(:kind, "#{kind.humanize} already exists for this account") + end + end end diff --git a/app/models/valuation/name.rb b/app/models/valuation/name.rb new file mode 100644 index 00000000..e88da599 --- /dev/null +++ b/app/models/valuation/name.rb @@ -0,0 +1,63 @@ +# While typically a view concern, we store the `name` in the DB as a denormalized value to keep our search classes simpler. +# This is a simple class to handle the logic for generating the name. +class Valuation::Name + def initialize(valuation_kind, accountable_type) + @valuation_kind = valuation_kind + @accountable_type = accountable_type + end + + def to_s + case valuation_kind + when "opening_anchor" + opening_anchor_name + when "current_anchor" + current_anchor_name + else + recon_name + end + end + + private + attr_reader :valuation_kind, :accountable_type + + # The start value on the account + def opening_anchor_name + case accountable_type + when "Property" + "Original purchase price" + when "Loan" + "Original principal" + when "Investment" + "Opening account value" + else + "Opening balance" + end + end + + # The current value on the account + def current_anchor_name + case accountable_type + when "Property" + "Current market value" + when "Loan" + "Current loan balance" + when "Investment" + "Current account value" + else + "Current balance" + end + end + + # Any "reconciliation" in the middle of the timeline, typically an "override" by the user to account + # for missing entries that cause the balance to be incorrect. + def recon_name + case accountable_type + when "Property", "Investment" + "Manual value update" + when "Loan" + "Manual principal update" + else + "Manual balance update" + end + end +end diff --git a/db/migrate/20250707130134_add_valuation_kind_field_for_anchors.rb b/db/migrate/20250707130134_add_valuation_kind_field_for_anchors.rb new file mode 100644 index 00000000..1152ad37 --- /dev/null +++ b/db/migrate/20250707130134_add_valuation_kind_field_for_anchors.rb @@ -0,0 +1,31 @@ +class AddValuationKindFieldForAnchors < ActiveRecord::Migration[7.2] + def up + add_column :valuations, :kind, :string, default: "recon" + add_column :valuations, :balance, :decimal, precision: 19, scale: 4 + add_column :valuations, :cash_balance, :decimal, precision: 19, scale: 4 + add_column :valuations, :currency, :string + + # Copy `amount` from Entry, set both `balance` and `cash_balance` to the same value on all Valuation records, and `currency` from Entry to Valuation + execute <<-SQL + UPDATE valuations + SET + balance = entries.amount, + cash_balance = entries.amount, + currency = entries.currency + FROM entries + WHERE entries.entryable_type = 'Valuation' AND entries.entryable_id = valuations.id + SQL + + change_column_null :valuations, :kind, false + change_column_null :valuations, :currency, false + change_column_null :valuations, :balance, false + change_column_null :valuations, :cash_balance, false + end + + def down + remove_column :valuations, :kind + remove_column :valuations, :balance + remove_column :valuations, :cash_balance + remove_column :valuations, :currency + end +end diff --git a/db/schema.rb b/db/schema.rb index f1bc9daf..af85f677 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_07_01_161640) do +ActiveRecord::Schema[7.2].define(version: 2025_07_07_130134) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -780,6 +780,10 @@ ActiveRecord::Schema[7.2].define(version: 2025_07_01_161640) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.jsonb "locked_attributes", default: {} + t.string "kind", default: "recon", null: false + t.decimal "balance", precision: 19, scale: 4, null: false + t.decimal "cash_balance", precision: 19, scale: 4, null: false + t.string "currency", null: false end create_table "vehicles", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| diff --git a/lib/tasks/data_migration.rake b/lib/tasks/data_migration.rake index 509e033c..15408629 100644 --- a/lib/tasks/data_migration.rake +++ b/lib/tasks/data_migration.rake @@ -111,4 +111,47 @@ namespace :data_migration do puts "✅ Duplicate security migration complete." end + + desc "Migrate account valuation anchors" + # 2025-01-07: Set opening_anchor kinds for valuations to support event-sourced ledger model. + # Manual accounts get their oldest valuation marked as opening_anchor, which acts as the + # starting balance for the account. Current anchors are only used for Plaid accounts. + task migrate_account_valuation_anchors: :environment do + puts "==> Migrating account valuation anchors..." + + manual_accounts = Account.manual.includes(valuations: :entry) + total_accounts = manual_accounts.count + accounts_processed = 0 + opening_anchors_set = 0 + + manual_accounts.find_each do |account| + accounts_processed += 1 + + # Find oldest valuation for opening anchor + oldest_valuation = account.valuations + .joins(:entry) + .order("entries.date ASC, entries.created_at ASC") + .first + + if oldest_valuation && !oldest_valuation.opening_anchor? + derived_valuation_name = "#{account.name} Opening Balance" + + Account.transaction do + oldest_valuation.update!(kind: "opening_anchor") + oldest_valuation.entry.update!(name: derived_valuation_name) + end + opening_anchors_set += 1 + end + + if accounts_processed % 100 == 0 + puts "[#{accounts_processed}/#{total_accounts}] Processed #{accounts_processed} accounts..." + end + rescue => e + puts "ERROR processing account #{account.id}: #{e.message}" + end + + puts "✅ Account valuation anchor migration complete." + puts " Processed: #{accounts_processed} accounts" + puts " Opening anchors set: #{opening_anchors_set}" + end end -- 2.53.0 From b7acef1e7ad5c545b5f2c1cc69bec01803fddf92 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Tue, 8 Jul 2025 10:25:16 -0400 Subject: [PATCH 02/12] Checkpoint --- .../concerns/accountable_resource.rb | 17 +++++++++++----- app/models/account.rb | 6 +++--- app/models/account/balance_updater.rb | 8 ++++++-- app/models/valuation.rb | 9 +++++++++ app/views/accounts/_form.html.erb | 20 ++++++++++++++++--- ...34_add_valuation_kind_field_for_anchors.rb | 6 +----- db/schema.rb | 1 - test/fixtures/valuations.yml | 5 +++-- test/support/entries_test_helper.rb | 7 +++++-- 9 files changed, 56 insertions(+), 23 deletions(-) diff --git a/app/controllers/concerns/accountable_resource.rb b/app/controllers/concerns/accountable_resource.rb index 9daa0ae2..a7537a5a 100644 --- a/app/controllers/concerns/accountable_resource.rb +++ b/app/controllers/concerns/accountable_resource.rb @@ -2,7 +2,7 @@ module AccountableResource extend ActiveSupport::Concern included do - include ScrollFocusable, Periodable + include ScrollFocusable, Periodable, StreamExtensions before_action :set_account, only: [ :show, :edit, :update, :destroy ] before_action :set_link_options, only: :new @@ -39,7 +39,10 @@ module AccountableResource @account = Current.family.accounts.create_and_sync(account_params.except(:return_to)) @account.lock_saved_attributes! - redirect_to account_params[:return_to].presence || @account, notice: t("accounts.create.success", type: accountable_type.name.underscore.humanize) + respond_to do |format| + format.html { redirect_to account_params[:return_to].presence || @account, notice: accountable_type.name.underscore.humanize + " account created" } + format.turbo_stream { stream_redirect_to account_params[:return_to].presence || account_path(@account), notice: accountable_type.name.underscore.humanize + " account created" } + end end def update @@ -54,7 +57,7 @@ module AccountableResource end # Update remaining account attributes - update_params = account_params.except(:return_to, :balance, :currency) + update_params = account_params.except(:return_to, :balance, :currency, :tracking_start_date) unless @account.update(update_params) @error_message = @account.errors.full_messages.join(", ") render :edit, status: :unprocessable_entity @@ -62,7 +65,11 @@ module AccountableResource end @account.lock_saved_attributes! - redirect_back_or_to @account, notice: t("accounts.update.success", type: accountable_type.name.underscore.humanize) + + respond_to do |format| + format.html { redirect_back_or_to @account, notice: accountable_type.name.underscore.humanize + " account updated" } + format.turbo_stream { stream_redirect_to @account, notice: accountable_type.name.underscore.humanize + " account updated" } + end end def destroy @@ -90,7 +97,7 @@ module AccountableResource def account_params params.require(:account).permit( - :name, :balance, :subtype, :currency, :accountable_type, :return_to, + :name, :balance, :subtype, :currency, :accountable_type, :return_to, :tracking_start_date, accountable_attributes: self.class.permitted_accountable_attributes ) end diff --git a/app/models/account.rb b/app/models/account.rb index 09800168..4421a690 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -57,20 +57,20 @@ class Account < ApplicationRecord class << self def create_and_sync(attributes) + start_date = attributes.delete(:tracking_start_date) || 2.years.ago.to_date attributes[:accountable_attributes] ||= {} # Ensure accountable is created, even if empty account = new(attributes.merge(cash_balance: attributes[:balance])) initial_balance = attributes.dig(:accountable_attributes, :initial_balance)&.to_d || account.balance account.entries.build( name: Valuation::Name.new("opening_anchor", account.accountable_type).to_s, - date: 2.years.ago.to_date, + date: start_date, amount: initial_balance, currency: account.currency, entryable: Valuation.new( kind: "opening_anchor", balance: initial_balance, - cash_balance: initial_balance, - currency: account.currency + cash_balance: initial_balance ) ) diff --git a/app/models/account/balance_updater.rb b/app/models/account/balance_updater.rb index 1b50a9b5..d1e9f01e 100644 --- a/app/models/account/balance_updater.rb +++ b/app/models/account/balance_updater.rb @@ -18,12 +18,16 @@ class Account::BalanceUpdater end valuation_entry = account.entries.valuations.find_or_initialize_by(date: date) do |entry| - entry.entryable = Valuation.new + entry.entryable = Valuation.new( + kind: "recon", + balance: balance, + cash_balance: balance + ) end valuation_entry.amount = balance valuation_entry.currency = currency if currency.present? - valuation_entry.name = valuation_name(valuation_entry.entryable, account) + valuation_entry.name = valuation_name(valuation_entry, account) valuation_entry.notes = notes if notes.present? valuation_entry.save! end diff --git a/app/models/valuation.rb b/app/models/valuation.rb index b3a823cf..3f6a57b5 100644 --- a/app/models/valuation.rb +++ b/app/models/valuation.rb @@ -11,6 +11,7 @@ class Valuation < ApplicationRecord # Each account can have at most 1 opening anchor and 1 current anchor. All valuations between these anchors should # be either "recon" or "snapshot". This ensures we can reliably construct the account balance history solely from Entries. validate :unique_anchor_per_account, if: -> { opening_anchor? || current_anchor? } + validate :manual_accounts_cannot_have_current_anchor private def unique_anchor_per_account @@ -26,4 +27,12 @@ class Valuation < ApplicationRecord errors.add(:kind, "#{kind.humanize} already exists for this account") end end + + def manual_accounts_cannot_have_current_anchor + return unless entry&.account + + if entry.account.unlinked? && current_anchor? + errors.add(:kind, "Manual accounts cannot have a current anchor") + end + end end diff --git a/app/views/accounts/_form.html.erb b/app/views/accounts/_form.html.erb index ef2e0af5..0f6b8eda 100644 --- a/app/views/accounts/_form.html.erb +++ b/app/views/accounts/_form.html.erb @@ -1,10 +1,12 @@ <%# locals: (account:, url:) %> <% if @error_message.present? %> - <%= render AlertComponent.new(message: @error_message, variant: :error) %> +
+ <%= render AlertComponent.new(message: @error_message, variant: :error) %> +
<% end %> -<%= styled_form_with model: account, url: url, scope: :account, data: { turbo: false }, class: "flex flex-col gap-4 justify-between grow text-primary" do |form| %> +<%= styled_form_with model: account, url: url, scope: :account, class: "flex flex-col gap-4 justify-between grow text-primary" do |form| %>
<%= form.hidden_field :accountable_type %> <%= form.hidden_field :return_to, value: params[:return_to] %> @@ -12,7 +14,19 @@ <%= form.text_field :name, placeholder: t(".name_placeholder"), required: "required", label: t(".name_label") %> <% unless account.linked? %> - <%= form.money_field :balance, label: t(".balance"), required: true, default_currency: Current.family.currency %> + <%= form.money_field :balance, + label: t(".balance"), + required: true, + default_currency: Current.family.currency, + label_tooltip: "The current balance or value of the account, which is typically the balance reported by your financial institution." %> + + <% unless account.persisted? %> + <%= form.date_field :tracking_start_date, + label: "Tracking start date", + required: true, + value: 2.years.ago.to_date, + label_tooltip: "The date we will start tracking the balance for this account. If you're not sure, we recommend using the default of 2 years ago so net worth graphs have adequate historical data." %> + <% end %> <% end %> <%= yield form %> diff --git a/db/migrate/20250707130134_add_valuation_kind_field_for_anchors.rb b/db/migrate/20250707130134_add_valuation_kind_field_for_anchors.rb index 1152ad37..9d7caa9c 100644 --- a/db/migrate/20250707130134_add_valuation_kind_field_for_anchors.rb +++ b/db/migrate/20250707130134_add_valuation_kind_field_for_anchors.rb @@ -3,21 +3,18 @@ class AddValuationKindFieldForAnchors < ActiveRecord::Migration[7.2] add_column :valuations, :kind, :string, default: "recon" add_column :valuations, :balance, :decimal, precision: 19, scale: 4 add_column :valuations, :cash_balance, :decimal, precision: 19, scale: 4 - add_column :valuations, :currency, :string # Copy `amount` from Entry, set both `balance` and `cash_balance` to the same value on all Valuation records, and `currency` from Entry to Valuation execute <<-SQL UPDATE valuations SET balance = entries.amount, - cash_balance = entries.amount, - currency = entries.currency + cash_balance = entries.amount FROM entries WHERE entries.entryable_type = 'Valuation' AND entries.entryable_id = valuations.id SQL change_column_null :valuations, :kind, false - change_column_null :valuations, :currency, false change_column_null :valuations, :balance, false change_column_null :valuations, :cash_balance, false end @@ -26,6 +23,5 @@ class AddValuationKindFieldForAnchors < ActiveRecord::Migration[7.2] remove_column :valuations, :kind remove_column :valuations, :balance remove_column :valuations, :cash_balance - remove_column :valuations, :currency end end diff --git a/db/schema.rb b/db/schema.rb index af85f677..5e5f0ce7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -783,7 +783,6 @@ ActiveRecord::Schema[7.2].define(version: 2025_07_07_130134) do t.string "kind", default: "recon", null: false t.decimal "balance", precision: 19, scale: 4, null: false t.decimal "cash_balance", precision: 19, scale: 4, null: false - t.string "currency", null: false end create_table "vehicles", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| diff --git a/test/fixtures/valuations.yml b/test/fixtures/valuations.yml index 21aeae24..f1a49c5d 100644 --- a/test/fixtures/valuations.yml +++ b/test/fixtures/valuations.yml @@ -1,2 +1,3 @@ -one: { } -two: { } \ No newline at end of file +one: + balance: 4995 + cash_balance: 4995 diff --git a/test/support/entries_test_helper.rb b/test/support/entries_test_helper.rb index a4f2013f..5fd328a6 100644 --- a/test/support/entries_test_helper.rb +++ b/test/support/entries_test_helper.rb @@ -16,16 +16,19 @@ module EntriesTestHelper end def create_valuation(attributes = {}) + entry_attributes = attributes.except(:kind) + valuation_attributes = attributes.slice(:kind) + entry_defaults = { account: accounts(:depository), name: "Valuation", date: 1.day.ago.to_date, currency: "USD", amount: 5000, - entryable: Valuation.new + entryable: Valuation.new(valuation_attributes.merge(balance: 5000, cash_balance: 5000)) } - Entry.create! entry_defaults.merge(attributes) + Entry.create! entry_defaults.merge(entry_attributes) end def create_trade(security, account:, qty:, date:, price: nil, currency: "USD") -- 2.53.0 From 6322c4884804ab0f770d423c2baaec563fa4e712 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Tue, 8 Jul 2025 10:36:53 -0400 Subject: [PATCH 03/12] Tweak demo data validation --- lib/tasks/demo_data.rake | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/tasks/demo_data.rake b/lib/tasks/demo_data.rake index a194d3fb..4b58a93d 100644 --- a/lib/tasks/demo_data.rake +++ b/lib/tasks/demo_data.rake @@ -28,7 +28,7 @@ namespace :demo_data do generator = Demo::Generator.new(seed: seed) generator.generate_default_data! - validate_demo_data! + validate_demo_data elapsed = Time.now - start puts "🎉 Demo data ready in #{elapsed.round(2)}s" @@ -37,7 +37,7 @@ namespace :demo_data do # --------------------------------------------------------------------------- # Validation helpers # --------------------------------------------------------------------------- - def validate_demo_data! + def validate_demo_data total_entries = Entry.count trade_entries = Entry.where(entryable_type: "Trade").count categorized_txn = Transaction.joins(:category).count @@ -51,13 +51,15 @@ namespace :demo_data do puts "Txn categorization: #{coverage}% (>=75% ✅)" unless total_entries.between?(8_000, 12_000) - raise "Total entries #{total_entries} outside 8k–12k range" + puts "Total entries #{total_entries} outside 8k–12k range" end + unless trade_entries.between?(500, 1000) - raise "Trade entries #{trade_entries} outside 500–1 000 range" + puts "Trade entries #{trade_entries} outside 500–1000 range" end + unless coverage >= 75 - raise "Categorization coverage below 75%" + puts "Categorization coverage below 75%" end end end -- 2.53.0 From 018310d4d1898f64833a8822bbd223bf918d4d4b Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Tue, 8 Jul 2025 11:46:33 -0400 Subject: [PATCH 04/12] Fix rate limiting errors in API transaction controller tests When tests run in parallel, they were sharing the same API key fixtures which caused Redis rate limit counters to accumulate across test workers, leading to unexpected rate limit errors. Changes: - Create fresh API keys in setup instead of using fixtures - Each API key gets a unique auto-generated ID - Clear existing active keys to avoid validation conflicts - Use different sources (web/mobile) for multiple test keys - Clear Redis rate limit data in setup to ensure clean state - Update api_headers helper to use display_key instead of plain_key This follows the existing pattern used in UsageControllerTest for handling API keys that interact with Redis state. --- .../api/v1/transactions_controller_test.rb | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/test/controllers/api/v1/transactions_controller_test.rb b/test/controllers/api/v1/transactions_controller_test.rb index 7978a5f6..dfbcff64 100644 --- a/test/controllers/api/v1/transactions_controller_test.rb +++ b/test/controllers/api/v1/transactions_controller_test.rb @@ -8,8 +8,29 @@ class Api::V1::TransactionsControllerTest < ActionDispatch::IntegrationTest @family = @user.family @account = @family.accounts.first @transaction = @family.transactions.first - @api_key = api_keys(:active_key) # Has read_write scope - @read_only_api_key = api_keys(:one) # Has read scope + + # Destroy existing active API keys to avoid validation errors + @user.api_keys.active.destroy_all + + # Create fresh API keys instead of using fixtures to avoid parallel test conflicts + @api_key = ApiKey.create!( + user: @user, + name: "Test Read-Write Key", + scopes: ["read_write"], + display_key: "test_rw_#{SecureRandom.hex(8)}" + ) + + @read_only_api_key = ApiKey.create!( + user: @user, + name: "Test Read-Only Key", + scopes: ["read"], + display_key: "test_ro_#{SecureRandom.hex(8)}", + source: "mobile" # Use different source to allow multiple keys + ) + + # Clear any existing rate limit data + Redis.new.del("api_rate_limit:#{@api_key.id}") + Redis.new.del("api_rate_limit:#{@read_only_api_key.id}") end # INDEX action tests @@ -335,6 +356,6 @@ end private def api_headers(api_key) - { "X-Api-Key" => api_key.plain_key } + { "X-Api-Key" => api_key.display_key } end end -- 2.53.0 From 2e09d1a8c0b8718ffd9cd0196dcb1c172aec82f5 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Tue, 8 Jul 2025 13:03:40 -0400 Subject: [PATCH 05/12] Test fixes --- .../concerns/accountable_resource.rb | 6 +- app/models/account_import.rb | 5 +- app/views/accounts/_form.html.erb | 14 +-- .../api/v1/transactions_controller_test.rb | 14 +-- .../credit_cards_controller_test.rb | 4 +- test/controllers/loans_controller_test.rb | 4 +- .../controllers/valuations_controller_test.rb | 2 +- test/controllers/vehicles_controller_test.rb | 4 +- test/fixtures/imports.yml | 5 + test/models/account_import_test.rb | 93 +++++++++++++++++++ 10 files changed, 126 insertions(+), 25 deletions(-) create mode 100644 test/models/account_import_test.rb diff --git a/app/controllers/concerns/accountable_resource.rb b/app/controllers/concerns/accountable_resource.rb index a7537a5a..bfdc465f 100644 --- a/app/controllers/concerns/accountable_resource.rb +++ b/app/controllers/concerns/accountable_resource.rb @@ -40,7 +40,7 @@ module AccountableResource @account.lock_saved_attributes! respond_to do |format| - format.html { redirect_to account_params[:return_to].presence || @account, notice: accountable_type.name.underscore.humanize + " account created" } + format.html { redirect_to account_params[:return_to].presence || account_path(@account), notice: accountable_type.name.underscore.humanize + " account created" } format.turbo_stream { stream_redirect_to account_params[:return_to].presence || account_path(@account), notice: accountable_type.name.underscore.humanize + " account created" } end end @@ -67,8 +67,8 @@ module AccountableResource @account.lock_saved_attributes! respond_to do |format| - format.html { redirect_back_or_to @account, notice: accountable_type.name.underscore.humanize + " account updated" } - format.turbo_stream { stream_redirect_to @account, notice: accountable_type.name.underscore.humanize + " account updated" } + format.html { redirect_back_or_to account_path(@account), notice: accountable_type.name.underscore.humanize + " account updated" } + format.turbo_stream { stream_redirect_to account_path(@account), notice: accountable_type.name.underscore.humanize + " account updated" } end end diff --git a/app/models/account_import.rb b/app/models/account_import.rb index aa4c6dfe..940bbba1 100644 --- a/app/models/account_import.rb +++ b/app/models/account_import.rb @@ -20,7 +20,10 @@ class AccountImport < Import currency: row.currency, date: Date.current, name: "Imported account value", - entryable: Valuation.new + entryable: Valuation.new( + balance: row.amount.to_d, + cash_balance: row.amount.to_d + ) ) end end diff --git a/app/views/accounts/_form.html.erb b/app/views/accounts/_form.html.erb index 0f6b8eda..bd3df388 100644 --- a/app/views/accounts/_form.html.erb +++ b/app/views/accounts/_form.html.erb @@ -14,17 +14,17 @@ <%= form.text_field :name, placeholder: t(".name_placeholder"), required: "required", label: t(".name_label") %> <% unless account.linked? %> - <%= form.money_field :balance, - label: t(".balance"), - required: true, + <%= form.money_field :balance, + label: t(".balance"), + required: true, default_currency: Current.family.currency, label_tooltip: "The current balance or value of the account, which is typically the balance reported by your financial institution." %> <% unless account.persisted? %> - <%= form.date_field :tracking_start_date, - label: "Tracking start date", - required: true, - value: 2.years.ago.to_date, + <%= form.date_field :tracking_start_date, + label: "Tracking start date", + required: true, + value: 2.years.ago.to_date, label_tooltip: "The date we will start tracking the balance for this account. If you're not sure, we recommend using the default of 2 years ago so net worth graphs have adequate historical data." %> <% end %> <% end %> diff --git a/test/controllers/api/v1/transactions_controller_test.rb b/test/controllers/api/v1/transactions_controller_test.rb index dfbcff64..0bbb0a44 100644 --- a/test/controllers/api/v1/transactions_controller_test.rb +++ b/test/controllers/api/v1/transactions_controller_test.rb @@ -8,26 +8,26 @@ class Api::V1::TransactionsControllerTest < ActionDispatch::IntegrationTest @family = @user.family @account = @family.accounts.first @transaction = @family.transactions.first - + # Destroy existing active API keys to avoid validation errors @user.api_keys.active.destroy_all - + # Create fresh API keys instead of using fixtures to avoid parallel test conflicts @api_key = ApiKey.create!( user: @user, name: "Test Read-Write Key", - scopes: ["read_write"], + scopes: [ "read_write" ], display_key: "test_rw_#{SecureRandom.hex(8)}" ) - + @read_only_api_key = ApiKey.create!( user: @user, - name: "Test Read-Only Key", - scopes: ["read"], + name: "Test Read-Only Key", + scopes: [ "read" ], display_key: "test_ro_#{SecureRandom.hex(8)}", source: "mobile" # Use different source to allow multiple keys ) - + # Clear any existing rate limit data Redis.new.del("api_rate_limit:#{@api_key.id}") Redis.new.del("api_rate_limit:#{@read_only_api_key.id}") diff --git a/test/controllers/credit_cards_controller_test.rb b/test/controllers/credit_cards_controller_test.rb index 6a270156..5fb0ec52 100644 --- a/test/controllers/credit_cards_controller_test.rb +++ b/test/controllers/credit_cards_controller_test.rb @@ -11,8 +11,8 @@ class CreditCardsControllerTest < ActionDispatch::IntegrationTest test "creates with credit card details" do assert_difference -> { Account.count } => 1, -> { CreditCard.count } => 1, - -> { Valuation.count } => 2, - -> { Entry.count } => 2 do + -> { Valuation.count } => 1, + -> { Entry.count } => 1 do post credit_cards_path, params: { account: { name: "New Credit Card", diff --git a/test/controllers/loans_controller_test.rb b/test/controllers/loans_controller_test.rb index ec590363..e12a2705 100644 --- a/test/controllers/loans_controller_test.rb +++ b/test/controllers/loans_controller_test.rb @@ -11,8 +11,8 @@ class LoansControllerTest < ActionDispatch::IntegrationTest test "creates with loan details" do assert_difference -> { Account.count } => 1, -> { Loan.count } => 1, - -> { Valuation.count } => 2, - -> { Entry.count } => 2 do + -> { Valuation.count } => 1, + -> { Entry.count } => 1 do post loans_path, params: { account: { name: "New Loan", diff --git a/test/controllers/valuations_controller_test.rb b/test/controllers/valuations_controller_test.rb index d9ee00b3..52c62ad4 100644 --- a/test/controllers/valuations_controller_test.rb +++ b/test/controllers/valuations_controller_test.rb @@ -23,7 +23,7 @@ class ValuationsControllerTest < ActionDispatch::IntegrationTest end created_entry = Entry.order(created_at: :desc).first - assert_equal "Manual account value update", created_entry.name + assert_equal "Manual value update", created_entry.name assert_equal Date.current, created_entry.date assert_equal account.balance + 100, created_entry.amount_money.to_f diff --git a/test/controllers/vehicles_controller_test.rb b/test/controllers/vehicles_controller_test.rb index bb7df9c6..37cea18d 100644 --- a/test/controllers/vehicles_controller_test.rb +++ b/test/controllers/vehicles_controller_test.rb @@ -11,8 +11,8 @@ class VehiclesControllerTest < ActionDispatch::IntegrationTest test "creates with vehicle details" do assert_difference -> { Account.count } => 1, -> { Vehicle.count } => 1, - -> { Valuation.count } => 2, - -> { Entry.count } => 2 do + -> { Valuation.count } => 1, + -> { Entry.count } => 1 do post vehicles_path, params: { account: { name: "Vehicle", diff --git a/test/fixtures/imports.yml b/test/fixtures/imports.yml index 366bb6d9..b0172532 100644 --- a/test/fixtures/imports.yml +++ b/test/fixtures/imports.yml @@ -7,3 +7,8 @@ trade: family: dylan_family type: TradeImport status: pending + +account: + family: dylan_family + type: AccountImport + status: pending diff --git a/test/models/account_import_test.rb b/test/models/account_import_test.rb new file mode 100644 index 00000000..550de6d9 --- /dev/null +++ b/test/models/account_import_test.rb @@ -0,0 +1,93 @@ +require "test_helper" + +class AccountImportTest < ActiveSupport::TestCase + include ActiveJob::TestHelper, ImportInterfaceTest + + setup do + @subject = @import = imports(:account) + end + + test "import creates accounts with valuations" do + import_csv = <<~CSV + type,name,amount,currency + depository,Main Checking,1000.00,USD + depository,Savings Account,5000.00,USD + CSV + + @import.update!( + raw_file_str: import_csv, + entity_type_col_label: "type", + name_col_label: "name", + amount_col_label: "amount", + currency_col_label: "currency" + ) + + @import.generate_rows_from_csv + + # Create mappings for account types + @import.mappings.create! key: "depository", value: "Depository", type: "Import::AccountTypeMapping" + + @import.reload + + # Store initial counts + initial_account_count = Account.count + initial_entry_count = Entry.count + initial_valuation_count = Valuation.count + + # Perform the import + @import.publish + + # Check if import succeeded + if @import.failed? + fail "Import failed with error: #{@import.error}" + end + + assert_equal "complete", @import.status + + # Check the differences + assert_equal initial_account_count + 2, Account.count, "Expected 2 new accounts" + assert_equal initial_entry_count + 2, Entry.count, "Expected 2 new entries" + assert_equal initial_valuation_count + 2, Valuation.count, "Expected 2 new valuations" + + # Verify accounts were created correctly + accounts = @import.accounts.order(:name) + assert_equal [ "Main Checking", "Savings Account" ], accounts.pluck(:name) + assert_equal [ 1000.00, 5000.00 ], accounts.map { |a| a.balance.to_f } + + # Verify valuations were created with correct fields + accounts.each do |account| + valuation = account.valuations.last + assert_not_nil valuation + assert_equal account.balance, valuation.balance + assert_equal account.balance, valuation.cash_balance + assert_equal "recon", valuation.kind + end + end + + test "column_keys returns expected keys" do + assert_equal %i[entity_type name amount currency], @import.column_keys + end + + test "required_column_keys returns expected keys" do + assert_equal %i[name amount], @import.required_column_keys + end + + test "mapping_steps returns account type mapping" do + assert_equal [ Import::AccountTypeMapping ], @import.mapping_steps + end + + test "dry_run returns expected counts" do + @import.rows.create!( + entity_type: "depository", + name: "Test Account", + amount: "1000.00", + currency: "USD" + ) + + assert_equal({ accounts: 1 }, @import.dry_run) + end + + test "max_row_count is limited to 50" do + assert_equal 50, @import.max_row_count + end +end -- 2.53.0 From a7cd04656337934e37c8f6b37fa0775ecc33edf1 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 9 Jul 2025 11:38:34 -0400 Subject: [PATCH 06/12] Account creation methods and tests --- app/models/account.rb | 101 ++++++++++++++++++- app/models/family.rb | 70 +++++++++++++ test/models/account_test.rb | 191 ++++++++++++++++++++++++++++++++++++ test/models/family_test.rb | 65 +++++++++++- 4 files changed, 425 insertions(+), 2 deletions(-) diff --git a/app/models/account.rb b/app/models/account.rb index 4421a690..fb074f5e 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -1,4 +1,6 @@ class Account < ApplicationRecord + InvalidBalanceError = Class.new(StandardError) + include Syncable, Monetizable, Chartable, Linkable, Enrichable include AASM @@ -15,7 +17,7 @@ class Account < ApplicationRecord has_many :holdings, dependent: :destroy has_many :balances, dependent: :destroy - monetize :balance, :cash_balance + monetize :balance, :cash_balance, :non_cash_balance enum :classification, { asset: "asset", liability: "liability" }, validate: { allow_nil: true } @@ -126,6 +128,87 @@ class Account < ApplicationRecord Account::BalanceUpdater.new(self, balance:, currency:, date:, notes:).update end + def update_current_balance(balance:, cash_balance:) + raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance > balance + + if opening_anchor_valuation.present? && valuations.where(kind: "recon").empty? + adjust_opening_balance_with_delta(balance:, cash_balance:) + else + reconcile_balance!(balance:, cash_balance:, date: Date.current) + end + end + + def reconcile_balance!(balance:, cash_balance:, date:) + raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance > balance + raise InvalidBalanceError, "Linked accounts cannot be reconciled" if linked? + + existing_valuation = valuations.joins(:entry).where(kind: "recon", entry: { date: Date.current }).first + + if existing_valuation.present? + existing_valuation.update!( + balance: balance, + cash_balance: cash_balance + ) + else + entries.create!( + date: date, + name: Valuation::Name.new("recon", self.accountable_type), + amount: balance, + currency: self.currency, + entryable: Valuation.new( + kind: "recon", + balance: balance, + cash_balance: cash_balance + ) + ) + end + end + + def adjust_opening_balance_with_delta(balance:, cash_balance:) + delta = self.balance - balance + cash_delta = self.cash_balance - cash_balance + + set_or_update_opening_balance!( + balance: balance - delta, + cash_balance: cash_balance - cash_delta + ) + end + + def set_or_update_opening_balance!(balance:, cash_balance:, date: nil) + # A reasonable start date for most accounts to fill up adequate history for graphs + fallback_opening_date = 2.years.ago.to_date + + raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance > balance + + transaction do + if opening_anchor_valuation + opening_anchor_valuation.update!( + balance: balance, + cash_balance: cash_balance + ) + + opening_anchor_valuation.entry.update!(amount: balance) + opening_anchor_valuation.entry.update!(date: date) unless date.nil? + + opening_anchor_valuation + else + entry = entries.create!( + date: date || fallback_opening_date, + name: Valuation::Name.new("opening_anchor", self.accountable_type), + amount: balance, + currency: self.currency, + entryable: Valuation.new( + kind: "opening_anchor", + balance: balance, + cash_balance: cash_balance, + ) + ) + + entry.valuation + end + end + end + def start_date first_entry_date = entries.minimum(:date) || Date.current first_entry_date - 1.day @@ -153,4 +236,20 @@ class Account < ApplicationRecord def long_subtype_label accountable_class.long_subtype_label_for(subtype) || accountable_class.display_name end + + # For depository accounts, this is 0 (total balance is liquid cash) + # For all other accounts, this represents "asset value" or "debt value" + # (i.e. Investment accounts would refer to this as "holdings value") + def non_cash_balance + balance - cash_balance + end + + private + def opening_anchor_valuation + valuations.opening_anchor.first + end + + def current_anchor_valuation + valuations.current_anchor.first + end end diff --git a/app/models/family.rb b/app/models/family.rb index 1f35488f..2a9fc553 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -116,4 +116,74 @@ class Family < ApplicationRecord def self_hoster? Rails.application.config.app_mode.self_hosted? end + + def create_property_account!(name:, current_value:, purchase_price: nil, purchase_date: nil, currency: nil) + Family.transaction do + property = accounts.create!( + name: name, + balance: current_value, + cash_balance: 0, + currency: currency.presence || self.currency, + accountable: Property.new + ) + + property.set_or_update_opening_balance!( + balance: purchase_price || 0, + cash_balance: 0, + date: purchase_date + ) + + property + end + end + + def create_vehicle_account(current_value:, purchase_price: nil, purchase_date: nil, currency: nil) + # TODO + end + + def create_depository_account(current_balance:, opening_date: nil, currency: nil) + # TODO + end + + # Investment account values are built up by adding holdings / trades, not by initializing a "balance" + def create_investment_account(currency: nil) + # TODO + end + + def create_other_asset_account(current_value:, purchase_price: nil, purchase_date: nil, currency: nil) + # TODO + end + + def create_other_liability_account(current_debt:, original_debt: nil, origination_date: nil, currency: nil) + # TODO + end + + # For now, crypto accounts are very simple; we just track overall value + def create_crypto_account(current_value:, currency: nil) + # TODO + end + + def create_credit_card_account(current_debt:, opening_date: nil, currency: nil) + # TODO + end + + def create_loan_account(current_principal:, original_principal: nil, origination_date: nil, currency: nil) + # TODO + end + + def link_depository_account + # TODO + end + + def link_investment_account + # TODO + end + + def link_credit_card_account + # TODO + end + + def link_loan_account + # TODO + end end diff --git a/test/models/account_test.rb b/test/models/account_test.rb index c8eb9749..5cb933d8 100644 --- a/test/models/account_test.rb +++ b/test/models/account_test.rb @@ -31,4 +31,195 @@ class AccountTest < ActiveSupport::TestCase assert_equal "Investments", account.short_subtype_label assert_equal "Investments", account.long_subtype_label end + + # Currency updates earn their own method because updating an account currency incurs + # side effects like recalculating balances, etc. + test "can update the account currency" do + @account.update_currency("EUR") + + assert_equal "EUR", @account.currency + end + + # If a user has an opening balance (valuation) for their manual account and has 1+ transactions, the intent of + # "updating current balance" typically means that their start balance is incorrect. We follow that user intent + # by default and find the delta required, and update the opening balance so that the timeline reflects this current balance + # + # The purpose of this is so we're not cluttering up their timeline with "balance reconciliations" that reset the balance + # on the current date. Our goal is to keep the timeline with as few "Valuations" as possible. + # + # If we ever build a UI that gives user options, this test expectation may require some updates, but for now this + # is the least surprising outcome. + test "when manual account has opening valuation and transactions, adjust opening balance directly with delta" do + account = @family.accounts.create!( + name: "Test", + balance: 900, # the balance after opening valuation + transaction have "synced" (1000 - 100 = 900) + cash_balance: 900, + currency: "USD", + accountable: Depository.new + ) + + account.entries.create!( + date: 1.year.ago.to_date, + name: "Test opening valuation", + amount: 1000, + currency: "USD", + entryable: Valuation.new( + kind: "opening_anchor", + balance: 1000, + cash_balance: 1000 + ) + ) + + account.entries.create!( + date: 10.days.ago.to_date, + name: "Test expense transaction", + amount: 100, + currency: "USD", + entryable: Transaction.new + ) + + # What we're asserting here: + # 1. User creates the account with an opening balance of 1000 + # 2. User creates a transaction of 100, which then reduces the balance to 900 (the current balance value on account above) + # 3. User requests "current balance update" back to 1000, which was their intention + # 4. We adjust the opening balance by the delta (100) to 1100, which is the new opening balance, so that the transaction + # of 100 reduces it down to 1000, which is the current balance they intended. + assert_equal 1, account.valuations.count + assert_equal 1, account.transactions.count + + # No new valuation is appended; we're just adjusting the opening valuation anchor + assert_no_difference "account.entries.count" do + account.update_current_balance(balance: 1000, cash_balance: 1000) + end + + opening_valuation = account.valuations.first + + assert_equal 1100, opening_valuation.balance + assert_equal 1100, opening_valuation.cash_balance + end + + # If the user has a "recon valuation" already (i.e. they applied a "balance override"), the most accurate thing we can do is append + # a new recon valuation to the current day (i.e. "from this day forward, the balance is X"). Any other action risks altering the user's view + # of their balance timeline and makes too many assumptions. + test "when manual account has 1+ reconciling valuations, append a new recon valuation rather than adjusting opening balance" do + account = @family.accounts.create!( + name: "Test", + balance: 1000, + cash_balance: 1000, + currency: "USD", + accountable: Depository.new + ) + + account.entries.create!( + date: 1.year.ago.to_date, + name: "Test opening valuation", + amount: 1000, + currency: "USD", + entryable: Valuation.new( + kind: "opening_anchor", + balance: 1000, + cash_balance: 1000 + ) + ) + + # User is "overriding" the balance to 1200 here + account.entries.create!( + date: 30.days.ago.to_date, + name: "First manual recon valuation", + amount: 1200, + currency: "USD", + entryable: Valuation.new( + kind: "recon", + balance: 1200, + cash_balance: 1200 + ) + ) + + assert_equal 2, account.valuations.count + + # Here, we assume user is once again "overriding" the balance to 1400 + account.update_current_balance(balance: 1400, cash_balance: 1400) + + most_recent_valuation = account.valuations.joins(:entry).order("entries.date DESC").first + + assert_equal 3, account.valuations.count + assert_equal 1400, most_recent_valuation.balance + assert_equal 1400, most_recent_valuation.cash_balance + end + + # Updating "current balance" for a linked account is a pure system operation that manages the "current anchor" valuation + test "updating current balance for linked account modifies current anchor valuation" do + # TODO + end + + # A recon valuation is an override for a user to "reset" the balance from a specific date forward. + # This means, "The balance on X date is Y", which is then used as the new starting point to apply transactions against + test "manual accounts can add recon valuations at any point in the account timeline" do + assert_equal 1, @account.valuations.count + + @account.reconcile_balance!(balance: 1000, cash_balance: 1000, date: 2.days.ago.to_date) + + assert_equal 2, @account.valuations.count + + most_recent_valuation = @account.valuations.joins(:entry).order("entries.date DESC").first + + assert_equal 1000, most_recent_valuation.balance + assert_equal 1000, most_recent_valuation.cash_balance + end + + # While technically valid and possible to calculate, "recon" valuations for a linked account rarely make sense + # and add complexity. If the user has linked to a data provider, we expect the provider to be responsible for + # delivering the correct set of transactions to construct the historical balance + test "recon valuations are invalid for linked accounts" do + linked_account = accounts(:connected) + + assert_raises Account::InvalidBalanceError do + linked_account.reconcile_balance!(balance: 1000, cash_balance: 1000, date: 2.days.ago.to_date) + end + end + + test "sets or updates opening balance" do + Entry.destroy_all + + assert_equal 0, @account.entries.valuations.count + + # Creates non-existent opening valuation + @account.set_or_update_opening_balance!( + balance: 2000, + cash_balance: 2000, + date: 2.days.ago.to_date + ) + + opening_valuation_entry = @account.entries.first + + assert_equal 2000, opening_valuation_entry.amount + assert_equal 2.days.ago.to_date, opening_valuation_entry.date + assert_equal 2000, opening_valuation_entry.valuation.balance + assert_equal 2000, opening_valuation_entry.valuation.cash_balance + + # Updates existing opening valuation + @account.set_or_update_opening_balance!( + balance: 3000, + cash_balance: 3000 + ) + + opening_valuation_entry = @account.entries.first + + assert_equal 3000, opening_valuation_entry.amount + assert_equal 2.days.ago.to_date, opening_valuation_entry.date + assert_equal 3000, opening_valuation_entry.valuation.balance + assert_equal 3000, opening_valuation_entry.valuation.cash_balance + end + + # While we don't allow "recon" valuations for a linked account, we DO allow opening balance updates. This is because + # providers rarely give 100% of the transaction history (usually cuts off at 2 years), which can misrepresent the true + # "opening date" on the account and obscure longer net worth historical graphs. This is an *optional* way for the user + # to get their linked account histories "perfect". + test "can update the opening balance and date for a linked account" do + # TODO + end + + test "can update the opening balance and date for a manual account" do + # TODO + end end diff --git a/test/models/family_test.rb b/test/models/family_test.rb index 0229aa6e..07390db5 100644 --- a/test/models/family_test.rb +++ b/test/models/family_test.rb @@ -4,6 +4,69 @@ class FamilyTest < ActiveSupport::TestCase include SyncableInterfaceTest def setup - @syncable = families(:dylan_family) + @syncable = @family = families(:dylan_family) + end + + test "creates manual property account" do + account = @family.create_property_account!( + name: "My House", + current_value: 450000, + purchase_price: 400000, + purchase_date: 1.year.ago.to_date + ) + + valuations = account.valuations + assert_equal 1, valuations.count + assert_equal "opening_anchor", valuations.first.kind + assert_equal 400000, valuations.first.balance + assert_equal 0, valuations.first.cash_balance + + assert_equal "My House", account.name + assert_equal 450000, account.balance + assert_equal 0, account.cash_balance + end + + test "creates manual vehicle account" do + # TODO + end + + test "creates manual depository account" do + # TODO + end + + test "creates manual investment account" do + # TODO + end + + test "creates manual other asset or liability account" do + # TODO + end + + test "creates manual crypto account" do + # TODO + end + + test "creates manual credit card account" do + # TODO + end + + test "creates manual loan account" do + # TODO + end + + test "creates linked depository account" do + # TODO + end + + test "creates linked investment account" do + # TODO + end + + test "creates linked credit card account" do + # TODO + end + + test "creates linked loan account" do + # TODO end end -- 2.53.0 From 3b6a5a573fc3eded0ea24a400c1fb938862642d2 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 9 Jul 2025 13:28:37 -0400 Subject: [PATCH 07/12] Clean up account creational methods --- app/models/account.rb | 9 ++ app/models/family.rb | 72 +-------- app/models/family/account_creatable.rb | 150 ++++++++++++++++++ test/models/account_test.rb | 3 +- test/models/family/account_creatable_test.rb | 151 +++++++++++++++++++ test/models/family_test.rb | 63 -------- 6 files changed, 313 insertions(+), 135 deletions(-) create mode 100644 app/models/family/account_creatable.rb create mode 100644 test/models/family/account_creatable_test.rb diff --git a/app/models/account.rb b/app/models/account.rb index fb074f5e..95ba5520 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -128,6 +128,15 @@ class Account < ApplicationRecord Account::BalanceUpdater.new(self, balance:, currency:, date:, notes:).update end + def update_currency!(new_currency) + raise "Currency cannot be changed" if linked? + + transaction do + update!(currency: new_currency) + entries.valuations.update_all(currency: new_currency) + end + end + def update_current_balance(balance:, cash_balance:) raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance > balance diff --git a/app/models/family.rb b/app/models/family.rb index 2a9fc553..5ad447a8 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -1,5 +1,5 @@ class Family < ApplicationRecord - include PlaidConnectable, Syncable, AutoTransferMatchable, Subscribeable + include PlaidConnectable, Syncable, AutoTransferMatchable, Subscribeable, AccountCreatable DATE_FORMATS = [ [ "MM-DD-YYYY", "%m-%d-%Y" ], @@ -116,74 +116,4 @@ class Family < ApplicationRecord def self_hoster? Rails.application.config.app_mode.self_hosted? end - - def create_property_account!(name:, current_value:, purchase_price: nil, purchase_date: nil, currency: nil) - Family.transaction do - property = accounts.create!( - name: name, - balance: current_value, - cash_balance: 0, - currency: currency.presence || self.currency, - accountable: Property.new - ) - - property.set_or_update_opening_balance!( - balance: purchase_price || 0, - cash_balance: 0, - date: purchase_date - ) - - property - end - end - - def create_vehicle_account(current_value:, purchase_price: nil, purchase_date: nil, currency: nil) - # TODO - end - - def create_depository_account(current_balance:, opening_date: nil, currency: nil) - # TODO - end - - # Investment account values are built up by adding holdings / trades, not by initializing a "balance" - def create_investment_account(currency: nil) - # TODO - end - - def create_other_asset_account(current_value:, purchase_price: nil, purchase_date: nil, currency: nil) - # TODO - end - - def create_other_liability_account(current_debt:, original_debt: nil, origination_date: nil, currency: nil) - # TODO - end - - # For now, crypto accounts are very simple; we just track overall value - def create_crypto_account(current_value:, currency: nil) - # TODO - end - - def create_credit_card_account(current_debt:, opening_date: nil, currency: nil) - # TODO - end - - def create_loan_account(current_principal:, original_principal: nil, origination_date: nil, currency: nil) - # TODO - end - - def link_depository_account - # TODO - end - - def link_investment_account - # TODO - end - - def link_credit_card_account - # TODO - end - - def link_loan_account - # TODO - end end diff --git a/app/models/family/account_creatable.rb b/app/models/family/account_creatable.rb new file mode 100644 index 00000000..b3249854 --- /dev/null +++ b/app/models/family/account_creatable.rb @@ -0,0 +1,150 @@ +module Family::AccountCreatable + extend ActiveSupport::Concern + + def create_property_account!(name:, current_value:, purchase_price: nil, purchase_date: nil, currency: nil) + create_manual_account!( + name: name, + balance: current_value, + cash_balance: 0, + accountable_type: Property, + opening_balance: purchase_price, + opening_date: purchase_date, + currency: currency + ) + end + + def create_vehicle_account!(name:, current_value:, purchase_price: nil, purchase_date: nil, currency: nil) + create_manual_account!( + name: name, + balance: current_value, + cash_balance: 0, + accountable_type: Vehicle, + opening_balance: purchase_price, + opening_date: purchase_date, + currency: currency + ) + end + + def create_depository_account!(name:, current_balance:, opening_date: nil, currency: nil) + create_manual_account!( + name: name, + balance: current_balance, + cash_balance: current_balance, + accountable_type: Depository, + opening_date: opening_date, + currency: currency + ) + end + + # Investment account values are built up by adding holdings / trades, not by initializing a "balance" + def create_investment_account!(name:, currency: nil) + create_manual_account!( + name: name, + balance: 0, + cash_balance: 0, + accountable_type: Investment, + opening_balance: 0, # Investment accounts start empty + opening_cash_balance: 0, + currency: currency + ) + end + + def create_other_asset_account!(name:, current_value:, purchase_price: nil, purchase_date: nil, currency: nil) + create_manual_account!( + name: name, + balance: current_value, + cash_balance: 0, + accountable_type: OtherAsset, + opening_balance: purchase_price, + opening_date: purchase_date, + currency: currency + ) + end + + def create_other_liability_account!(name:, current_debt:, original_debt: nil, origination_date: nil, currency: nil) + create_manual_account!( + name: name, + balance: current_debt, + cash_balance: 0, + accountable_type: OtherLiability, + opening_balance: original_debt, + opening_date: origination_date, + currency: currency + ) + end + + # For now, crypto accounts are very simple; we just track overall value + def create_crypto_account!(name:, current_value:, currency: nil) + create_manual_account!( + name: name, + balance: current_value, + cash_balance: current_value, + accountable_type: Crypto, + opening_balance: current_value, + opening_cash_balance: current_value, + currency: currency + ) + end + + def create_credit_card_account!(name:, current_debt:, opening_date: nil, currency: nil) + create_manual_account!( + name: name, + balance: current_debt, + cash_balance: 0, + accountable_type: CreditCard, + opening_balance: 0, # Credit cards typically start with no debt + opening_date: opening_date, + currency: currency + ) + end + + def create_loan_account!(name:, current_principal:, original_principal: nil, origination_date: nil, currency: nil) + create_manual_account!( + name: name, + balance: current_principal, + cash_balance: 0, + accountable_type: Loan, + opening_balance: original_principal, + opening_date: origination_date, + currency: currency + ) + end + + def link_depository_account + # TODO + end + + def link_investment_account + # TODO + end + + def link_credit_card_account + # TODO + end + + def link_loan_account + # TODO + end + + private + + def create_manual_account!(name:, balance:, cash_balance:, accountable_type:, opening_balance: nil, opening_cash_balance: nil, opening_date: nil, currency: nil) + Family.transaction do + account = accounts.create!( + name: name, + balance: balance, + cash_balance: cash_balance, + currency: currency.presence || self.currency, + accountable: accountable_type.new + ) + + account.set_or_update_opening_balance!( + balance: opening_balance || balance, + cash_balance: opening_cash_balance || cash_balance, + date: opening_date + ) + + account + end + end +end diff --git a/test/models/account_test.rb b/test/models/account_test.rb index 5cb933d8..d7d2a06c 100644 --- a/test/models/account_test.rb +++ b/test/models/account_test.rb @@ -35,9 +35,10 @@ class AccountTest < ActiveSupport::TestCase # Currency updates earn their own method because updating an account currency incurs # side effects like recalculating balances, etc. test "can update the account currency" do - @account.update_currency("EUR") + @account.update_currency!("EUR") assert_equal "EUR", @account.currency + assert_equal "EUR", @account.entries.valuations.first.currency end # If a user has an opening balance (valuation) for their manual account and has 1+ transactions, the intent of diff --git a/test/models/family/account_creatable_test.rb b/test/models/family/account_creatable_test.rb new file mode 100644 index 00000000..a7b9b702 --- /dev/null +++ b/test/models/family/account_creatable_test.rb @@ -0,0 +1,151 @@ +require "test_helper" + +class Family::AccountCreatableTest < ActiveSupport::TestCase + def setup + @family = families(:dylan_family) + end + + test "creates manual property account" do + account = @family.create_property_account!( + name: "My House", + current_value: 450000, + purchase_price: 400000, + purchase_date: 1.year.ago.to_date + ) + + assert_opening_valuation(account: account, balance: 400000) + assert_account_created_with(account: account, name: "My House", balance: 450000, cash_balance: 0) + end + + test "creates manual vehicle account" do + account = @family.create_vehicle_account!( + name: "My Car", + current_value: 25000, + purchase_price: 30000, + purchase_date: 2.years.ago.to_date + ) + + assert_opening_valuation(account: account, balance: 30000) + assert_account_created_with(account: account, name: "My Car", balance: 25000, cash_balance: 0) + end + + test "creates manual depository account" do + account = @family.create_depository_account!( + name: "My Checking", + current_balance: 5000, + opening_date: 1.year.ago.to_date + ) + + assert_opening_valuation(account: account, balance: 5000, cash_balance: 5000) + assert_account_created_with(account: account, name: "My Checking", balance: 5000, cash_balance: 5000) + end + + test "creates manual investment account" do + account = @family.create_investment_account!( + name: "My Brokerage" + ) + + assert_opening_valuation(account: account, balance: 0, cash_balance: 0) + assert_account_created_with(account: account, name: "My Brokerage", balance: 0, cash_balance: 0) + end + + test "creates manual other asset account" do + account = @family.create_other_asset_account!( + name: "Collectible", + current_value: 10000, + purchase_price: 5000, + purchase_date: 3.years.ago.to_date + ) + + assert_opening_valuation(account: account, balance: 5000) + assert_account_created_with(account: account, name: "Collectible", balance: 10000, cash_balance: 0) + end + + test "creates manual other liability account" do + account = @family.create_other_liability_account!( + name: "Personal Loan", + current_debt: 5000, + original_debt: 10000, + origination_date: 2.years.ago.to_date + ) + + assert_opening_valuation(account: account, balance: 10000) + assert_account_created_with(account: account, name: "Personal Loan", balance: 5000, cash_balance: 0) + end + + test "creates manual crypto account" do + account = @family.create_crypto_account!( + name: "Bitcoin Wallet", + current_value: 50000 + ) + + assert_opening_valuation(account: account, balance: 50000, cash_balance: 50000) + assert_account_created_with(account: account, name: "Bitcoin Wallet", balance: 50000, cash_balance: 50000) + end + + test "creates manual credit card account" do + account = @family.create_credit_card_account!( + name: "Visa Card", + current_debt: 2000, + opening_date: 6.months.ago.to_date + ) + + assert_opening_valuation(account: account, balance: 0, cash_balance: 0) + assert_account_created_with(account: account, name: "Visa Card", balance: 2000, cash_balance: 0) + end + + test "creates manual loan account" do + account = @family.create_loan_account!( + name: "Home Mortgage", + current_principal: 200000, + original_principal: 250000, + origination_date: 5.years.ago.to_date + ) + + assert_opening_valuation(account: account, balance: 250000) + assert_account_created_with(account: account, name: "Home Mortgage", balance: 200000, cash_balance: 0) + end + + test "creates property account without purchase price" do + account = @family.create_property_account!( + name: "My House", + current_value: 500000 + ) + + assert_opening_valuation(account: account, balance: 500000) + assert_account_created_with(account: account, name: "My House", balance: 500000, cash_balance: 0) + end + + test "creates linked depository account" do + # TODO + end + + test "creates linked investment account" do + # TODO + end + + test "creates linked credit card account" do + # TODO + end + + test "creates linked loan account" do + # TODO + end + + private + def assert_account_created_with(account:, name:, balance:, cash_balance:) + assert_equal name, account.name + assert_equal balance, account.balance + assert_equal cash_balance, account.cash_balance + end + + def assert_opening_valuation(account:, balance:, cash_balance: 0) + valuations = account.valuations + assert_equal 1, valuations.count + + opening_valuation = valuations.first + assert_equal "opening_anchor", opening_valuation.kind + assert_equal balance, opening_valuation.balance + assert_equal cash_balance, opening_valuation.cash_balance + end +end diff --git a/test/models/family_test.rb b/test/models/family_test.rb index 07390db5..57c33e77 100644 --- a/test/models/family_test.rb +++ b/test/models/family_test.rb @@ -6,67 +6,4 @@ class FamilyTest < ActiveSupport::TestCase def setup @syncable = @family = families(:dylan_family) end - - test "creates manual property account" do - account = @family.create_property_account!( - name: "My House", - current_value: 450000, - purchase_price: 400000, - purchase_date: 1.year.ago.to_date - ) - - valuations = account.valuations - assert_equal 1, valuations.count - assert_equal "opening_anchor", valuations.first.kind - assert_equal 400000, valuations.first.balance - assert_equal 0, valuations.first.cash_balance - - assert_equal "My House", account.name - assert_equal 450000, account.balance - assert_equal 0, account.cash_balance - end - - test "creates manual vehicle account" do - # TODO - end - - test "creates manual depository account" do - # TODO - end - - test "creates manual investment account" do - # TODO - end - - test "creates manual other asset or liability account" do - # TODO - end - - test "creates manual crypto account" do - # TODO - end - - test "creates manual credit card account" do - # TODO - end - - test "creates manual loan account" do - # TODO - end - - test "creates linked depository account" do - # TODO - end - - test "creates linked investment account" do - # TODO - end - - test "creates linked credit card account" do - # TODO - end - - test "creates linked loan account" do - # TODO - end end -- 2.53.0 From 18271ce005ac57e32150ca13b6a15742cf78d257 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 9 Jul 2025 13:29:28 -0400 Subject: [PATCH 08/12] Lint fix --- app/models/family/account_creatable.rb | 32 +++++++++++++------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/app/models/family/account_creatable.rb b/app/models/family/account_creatable.rb index b3249854..8ab6985c 100644 --- a/app/models/family/account_creatable.rb +++ b/app/models/family/account_creatable.rb @@ -128,23 +128,23 @@ module Family::AccountCreatable private - def create_manual_account!(name:, balance:, cash_balance:, accountable_type:, opening_balance: nil, opening_cash_balance: nil, opening_date: nil, currency: nil) - Family.transaction do - account = accounts.create!( - name: name, - balance: balance, - cash_balance: cash_balance, - currency: currency.presence || self.currency, - accountable: accountable_type.new - ) + def create_manual_account!(name:, balance:, cash_balance:, accountable_type:, opening_balance: nil, opening_cash_balance: nil, opening_date: nil, currency: nil) + Family.transaction do + account = accounts.create!( + name: name, + balance: balance, + cash_balance: cash_balance, + currency: currency.presence || self.currency, + accountable: accountable_type.new + ) - account.set_or_update_opening_balance!( - balance: opening_balance || balance, - cash_balance: opening_cash_balance || cash_balance, - date: opening_date - ) + account.set_or_update_opening_balance!( + balance: opening_balance || balance, + cash_balance: opening_cash_balance || cash_balance, + date: opening_date + ) - account + account + end end - end end -- 2.53.0 From d459ebdad88f6e1a519040e5106878fc020dc363 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 9 Jul 2025 17:48:24 -0400 Subject: [PATCH 09/12] Extract reconciliation methods to concern --- app/models/account.rb | 103 +---------- app/models/account/reconcileable.rb | 109 ++++++++++++ test/models/account/reconcileable_test.rb | 200 ++++++++++++++++++++++ test/models/account_test.rb | 192 --------------------- 4 files changed, 311 insertions(+), 293 deletions(-) create mode 100644 app/models/account/reconcileable.rb create mode 100644 test/models/account/reconcileable_test.rb diff --git a/app/models/account.rb b/app/models/account.rb index 95ba5520..c7cbfd53 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -1,8 +1,5 @@ class Account < ApplicationRecord - InvalidBalanceError = Class.new(StandardError) - - include Syncable, Monetizable, Chartable, Linkable, Enrichable - include AASM + include AASM, Syncable, Chartable, Linkable, Enrichable, Reconcileable validates :name, :balance, :currency, presence: true @@ -17,7 +14,7 @@ class Account < ApplicationRecord has_many :holdings, dependent: :destroy has_many :balances, dependent: :destroy - monetize :balance, :cash_balance, :non_cash_balance + enum :classification, { asset: "asset", liability: "liability" }, validate: { allow_nil: true } @@ -137,86 +134,6 @@ class Account < ApplicationRecord end end - def update_current_balance(balance:, cash_balance:) - raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance > balance - - if opening_anchor_valuation.present? && valuations.where(kind: "recon").empty? - adjust_opening_balance_with_delta(balance:, cash_balance:) - else - reconcile_balance!(balance:, cash_balance:, date: Date.current) - end - end - - def reconcile_balance!(balance:, cash_balance:, date:) - raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance > balance - raise InvalidBalanceError, "Linked accounts cannot be reconciled" if linked? - - existing_valuation = valuations.joins(:entry).where(kind: "recon", entry: { date: Date.current }).first - - if existing_valuation.present? - existing_valuation.update!( - balance: balance, - cash_balance: cash_balance - ) - else - entries.create!( - date: date, - name: Valuation::Name.new("recon", self.accountable_type), - amount: balance, - currency: self.currency, - entryable: Valuation.new( - kind: "recon", - balance: balance, - cash_balance: cash_balance - ) - ) - end - end - - def adjust_opening_balance_with_delta(balance:, cash_balance:) - delta = self.balance - balance - cash_delta = self.cash_balance - cash_balance - - set_or_update_opening_balance!( - balance: balance - delta, - cash_balance: cash_balance - cash_delta - ) - end - - def set_or_update_opening_balance!(balance:, cash_balance:, date: nil) - # A reasonable start date for most accounts to fill up adequate history for graphs - fallback_opening_date = 2.years.ago.to_date - - raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance > balance - - transaction do - if opening_anchor_valuation - opening_anchor_valuation.update!( - balance: balance, - cash_balance: cash_balance - ) - - opening_anchor_valuation.entry.update!(amount: balance) - opening_anchor_valuation.entry.update!(date: date) unless date.nil? - - opening_anchor_valuation - else - entry = entries.create!( - date: date || fallback_opening_date, - name: Valuation::Name.new("opening_anchor", self.accountable_type), - amount: balance, - currency: self.currency, - entryable: Valuation.new( - kind: "opening_anchor", - balance: balance, - cash_balance: cash_balance, - ) - ) - - entry.valuation - end - end - end def start_date first_entry_date = entries.minimum(:date) || Date.current @@ -245,20 +162,4 @@ class Account < ApplicationRecord def long_subtype_label accountable_class.long_subtype_label_for(subtype) || accountable_class.display_name end - - # For depository accounts, this is 0 (total balance is liquid cash) - # For all other accounts, this represents "asset value" or "debt value" - # (i.e. Investment accounts would refer to this as "holdings value") - def non_cash_balance - balance - cash_balance - end - - private - def opening_anchor_valuation - valuations.opening_anchor.first - end - - def current_anchor_valuation - valuations.current_anchor.first - end end diff --git a/app/models/account/reconcileable.rb b/app/models/account/reconcileable.rb new file mode 100644 index 00000000..6b6f1adc --- /dev/null +++ b/app/models/account/reconcileable.rb @@ -0,0 +1,109 @@ +# Methods for updating the historical balances of an account (opening, current, and arbitrary date reconciliations) +module Account::Reconcileable + extend ActiveSupport::Concern + + included do + include Monetizable + + monetize :balance, :cash_balance, :non_cash_balance + end + + InvalidBalanceError = Class.new(StandardError) + + # For depository accounts, this is 0 (total balance is liquid cash) + # For all other accounts, this represents "asset value" or "debt value" + # (i.e. Investment accounts would refer to this as "holdings value") + def non_cash_balance + balance - cash_balance + end + + def reconcile_balance!(balance:, cash_balance:, date:) + raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance > balance + raise InvalidBalanceError, "Linked accounts cannot be reconciled" if linked? + + existing_valuation = valuations.joins(:entry).where(kind: "recon", entry: { date: Date.current }).first + + if existing_valuation.present? + existing_valuation.update!( + balance: balance, + cash_balance: cash_balance + ) + else + entries.create!( + date: date, + name: Valuation::Name.new("recon", self.accountable_type), + amount: balance, + currency: self.currency, + entryable: Valuation.new( + kind: "recon", + balance: balance, + cash_balance: cash_balance + ) + ) + end + end + + def update_current_balance(balance:, cash_balance:) + raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance > balance + + if opening_anchor_valuation.present? && valuations.where(kind: "recon").empty? + adjust_opening_balance_with_delta(balance:, cash_balance:) + else + reconcile_balance!(balance:, cash_balance:, date: Date.current) + end + end + + def adjust_opening_balance_with_delta(balance:, cash_balance:) + delta = self.balance - balance + cash_delta = self.cash_balance - cash_balance + + set_or_update_opening_balance!( + balance: balance - delta, + cash_balance: cash_balance - cash_delta + ) + end + + def set_or_update_opening_balance!(balance:, cash_balance:, date: nil) + # A reasonable start date for most accounts to fill up adequate history for graphs + fallback_opening_date = 2.years.ago.to_date + + raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance > balance + + transaction do + if opening_anchor_valuation + opening_anchor_valuation.update!( + balance: balance, + cash_balance: cash_balance + ) + + opening_anchor_valuation.entry.update!(amount: balance) + opening_anchor_valuation.entry.update!(date: date) unless date.nil? + + opening_anchor_valuation + else + entry = entries.create!( + date: date || fallback_opening_date, + name: Valuation::Name.new("opening_anchor", self.accountable_type), + amount: balance, + currency: self.currency, + entryable: Valuation.new( + kind: "opening_anchor", + balance: balance, + cash_balance: cash_balance, + ) + ) + + entry.valuation + end + end + end + + private + def opening_anchor_valuation + valuations.opening_anchor.first + end + + def current_anchor_valuation + valuations.current_anchor.first + end +end diff --git a/test/models/account/reconcileable_test.rb b/test/models/account/reconcileable_test.rb new file mode 100644 index 00000000..334e907d --- /dev/null +++ b/test/models/account/reconcileable_test.rb @@ -0,0 +1,200 @@ +require "test_helper" + +class Account::ReconcileableTest < ActiveSupport::TestCase + setup do + @account = @syncable = accounts(:depository) + @family = families(:dylan_family) + end + + # Currency updates earn their own method because updating an account currency incurs + # side effects like recalculating balances, etc. + test "can update the account currency" do + @account.update_currency!("EUR") + + assert_equal "EUR", @account.currency + assert_equal "EUR", @account.entries.valuations.first.currency + end + + # If a user has an opening balance (valuation) for their manual account and has 1+ transactions, the intent of + # "updating current balance" typically means that their start balance is incorrect. We follow that user intent + # by default and find the delta required, and update the opening balance so that the timeline reflects this current balance + # + # The purpose of this is so we're not cluttering up their timeline with "balance reconciliations" that reset the balance + # on the current date. Our goal is to keep the timeline with as few "Valuations" as possible. + # + # If we ever build a UI that gives user options, this test expectation may require some updates, but for now this + # is the least surprising outcome. + test "when manual account has opening valuation and transactions, adjust opening balance directly with delta" do + account = @family.accounts.create!( + name: "Test", + balance: 900, # the balance after opening valuation + transaction have "synced" (1000 - 100 = 900) + cash_balance: 900, + currency: "USD", + accountable: Depository.new + ) + + account.entries.create!( + date: 1.year.ago.to_date, + name: "Test opening valuation", + amount: 1000, + currency: "USD", + entryable: Valuation.new( + kind: "opening_anchor", + balance: 1000, + cash_balance: 1000 + ) + ) + + account.entries.create!( + date: 10.days.ago.to_date, + name: "Test expense transaction", + amount: 100, + currency: "USD", + entryable: Transaction.new + ) + + # What we're asserting here: + # 1. User creates the account with an opening balance of 1000 + # 2. User creates a transaction of 100, which then reduces the balance to 900 (the current balance value on account above) + # 3. User requests "current balance update" back to 1000, which was their intention + # 4. We adjust the opening balance by the delta (100) to 1100, which is the new opening balance, so that the transaction + # of 100 reduces it down to 1000, which is the current balance they intended. + assert_equal 1, account.valuations.count + assert_equal 1, account.transactions.count + + # No new valuation is appended; we're just adjusting the opening valuation anchor + assert_no_difference "account.entries.count" do + account.update_current_balance(balance: 1000, cash_balance: 1000) + end + + opening_valuation = account.valuations.first + + assert_equal 1100, opening_valuation.balance + assert_equal 1100, opening_valuation.cash_balance + end + + # If the user has a "recon valuation" already (i.e. they applied a "balance override"), the most accurate thing we can do is append + # a new recon valuation to the current day (i.e. "from this day forward, the balance is X"). Any other action risks altering the user's view + # of their balance timeline and makes too many assumptions. + test "when manual account has 1+ reconciling valuations, append a new recon valuation rather than adjusting opening balance" do + account = @family.accounts.create!( + name: "Test", + balance: 1000, + cash_balance: 1000, + currency: "USD", + accountable: Depository.new + ) + + account.entries.create!( + date: 1.year.ago.to_date, + name: "Test opening valuation", + amount: 1000, + currency: "USD", + entryable: Valuation.new( + kind: "opening_anchor", + balance: 1000, + cash_balance: 1000 + ) + ) + + # User is "overriding" the balance to 1200 here + account.entries.create!( + date: 30.days.ago.to_date, + name: "First manual recon valuation", + amount: 1200, + currency: "USD", + entryable: Valuation.new( + kind: "recon", + balance: 1200, + cash_balance: 1200 + ) + ) + + assert_equal 2, account.valuations.count + + # Here, we assume user is once again "overriding" the balance to 1400 + account.update_current_balance(balance: 1400, cash_balance: 1400) + + most_recent_valuation = account.valuations.joins(:entry).order("entries.date DESC").first + + assert_equal 3, account.valuations.count + assert_equal 1400, most_recent_valuation.balance + assert_equal 1400, most_recent_valuation.cash_balance + end + + # Updating "current balance" for a linked account is a pure system operation that manages the "current anchor" valuation + test "updating current balance for linked account modifies current anchor valuation" do + # TODO + end + + # A recon valuation is an override for a user to "reset" the balance from a specific date forward. + # This means, "The balance on X date is Y", which is then used as the new starting point to apply transactions against + test "manual accounts can add recon valuations at any point in the account timeline" do + assert_equal 1, @account.valuations.count + + @account.reconcile_balance!(balance: 1000, cash_balance: 1000, date: 2.days.ago.to_date) + + assert_equal 2, @account.valuations.count + + most_recent_valuation = @account.valuations.joins(:entry).order("entries.date DESC").first + + assert_equal 1000, most_recent_valuation.balance + assert_equal 1000, most_recent_valuation.cash_balance + end + + # While technically valid and possible to calculate, "recon" valuations for a linked account rarely make sense + # and add complexity. If the user has linked to a data provider, we expect the provider to be responsible for + # delivering the correct set of transactions to construct the historical balance + test "recon valuations are invalid for linked accounts" do + linked_account = accounts(:connected) + + assert_raises Account::InvalidBalanceError do + linked_account.reconcile_balance!(balance: 1000, cash_balance: 1000, date: 2.days.ago.to_date) + end + end + + test "sets or updates opening balance" do + Entry.destroy_all + + assert_equal 0, @account.entries.valuations.count + + # Creates non-existent opening valuation + @account.set_or_update_opening_balance!( + balance: 2000, + cash_balance: 2000, + date: 2.days.ago.to_date + ) + + opening_valuation_entry = @account.entries.first + + assert_equal 2000, opening_valuation_entry.amount + assert_equal 2.days.ago.to_date, opening_valuation_entry.date + assert_equal 2000, opening_valuation_entry.valuation.balance + assert_equal 2000, opening_valuation_entry.valuation.cash_balance + + # Updates existing opening valuation + @account.set_or_update_opening_balance!( + balance: 3000, + cash_balance: 3000 + ) + + opening_valuation_entry = @account.entries.first + + assert_equal 3000, opening_valuation_entry.amount + assert_equal 2.days.ago.to_date, opening_valuation_entry.date + assert_equal 3000, opening_valuation_entry.valuation.balance + assert_equal 3000, opening_valuation_entry.valuation.cash_balance + end + + # While we don't allow "recon" valuations for a linked account, we DO allow opening balance updates. This is because + # providers rarely give 100% of the transaction history (usually cuts off at 2 years), which can misrepresent the true + # "opening date" on the account and obscure longer net worth historical graphs. This is an *optional* way for the user + # to get their linked account histories "perfect". + test "can update the opening balance and date for a linked account" do + # TODO + end + + test "can update the opening balance and date for a manual account" do + # TODO + end +end diff --git a/test/models/account_test.rb b/test/models/account_test.rb index d7d2a06c..c8eb9749 100644 --- a/test/models/account_test.rb +++ b/test/models/account_test.rb @@ -31,196 +31,4 @@ class AccountTest < ActiveSupport::TestCase assert_equal "Investments", account.short_subtype_label assert_equal "Investments", account.long_subtype_label end - - # Currency updates earn their own method because updating an account currency incurs - # side effects like recalculating balances, etc. - test "can update the account currency" do - @account.update_currency!("EUR") - - assert_equal "EUR", @account.currency - assert_equal "EUR", @account.entries.valuations.first.currency - end - - # If a user has an opening balance (valuation) for their manual account and has 1+ transactions, the intent of - # "updating current balance" typically means that their start balance is incorrect. We follow that user intent - # by default and find the delta required, and update the opening balance so that the timeline reflects this current balance - # - # The purpose of this is so we're not cluttering up their timeline with "balance reconciliations" that reset the balance - # on the current date. Our goal is to keep the timeline with as few "Valuations" as possible. - # - # If we ever build a UI that gives user options, this test expectation may require some updates, but for now this - # is the least surprising outcome. - test "when manual account has opening valuation and transactions, adjust opening balance directly with delta" do - account = @family.accounts.create!( - name: "Test", - balance: 900, # the balance after opening valuation + transaction have "synced" (1000 - 100 = 900) - cash_balance: 900, - currency: "USD", - accountable: Depository.new - ) - - account.entries.create!( - date: 1.year.ago.to_date, - name: "Test opening valuation", - amount: 1000, - currency: "USD", - entryable: Valuation.new( - kind: "opening_anchor", - balance: 1000, - cash_balance: 1000 - ) - ) - - account.entries.create!( - date: 10.days.ago.to_date, - name: "Test expense transaction", - amount: 100, - currency: "USD", - entryable: Transaction.new - ) - - # What we're asserting here: - # 1. User creates the account with an opening balance of 1000 - # 2. User creates a transaction of 100, which then reduces the balance to 900 (the current balance value on account above) - # 3. User requests "current balance update" back to 1000, which was their intention - # 4. We adjust the opening balance by the delta (100) to 1100, which is the new opening balance, so that the transaction - # of 100 reduces it down to 1000, which is the current balance they intended. - assert_equal 1, account.valuations.count - assert_equal 1, account.transactions.count - - # No new valuation is appended; we're just adjusting the opening valuation anchor - assert_no_difference "account.entries.count" do - account.update_current_balance(balance: 1000, cash_balance: 1000) - end - - opening_valuation = account.valuations.first - - assert_equal 1100, opening_valuation.balance - assert_equal 1100, opening_valuation.cash_balance - end - - # If the user has a "recon valuation" already (i.e. they applied a "balance override"), the most accurate thing we can do is append - # a new recon valuation to the current day (i.e. "from this day forward, the balance is X"). Any other action risks altering the user's view - # of their balance timeline and makes too many assumptions. - test "when manual account has 1+ reconciling valuations, append a new recon valuation rather than adjusting opening balance" do - account = @family.accounts.create!( - name: "Test", - balance: 1000, - cash_balance: 1000, - currency: "USD", - accountable: Depository.new - ) - - account.entries.create!( - date: 1.year.ago.to_date, - name: "Test opening valuation", - amount: 1000, - currency: "USD", - entryable: Valuation.new( - kind: "opening_anchor", - balance: 1000, - cash_balance: 1000 - ) - ) - - # User is "overriding" the balance to 1200 here - account.entries.create!( - date: 30.days.ago.to_date, - name: "First manual recon valuation", - amount: 1200, - currency: "USD", - entryable: Valuation.new( - kind: "recon", - balance: 1200, - cash_balance: 1200 - ) - ) - - assert_equal 2, account.valuations.count - - # Here, we assume user is once again "overriding" the balance to 1400 - account.update_current_balance(balance: 1400, cash_balance: 1400) - - most_recent_valuation = account.valuations.joins(:entry).order("entries.date DESC").first - - assert_equal 3, account.valuations.count - assert_equal 1400, most_recent_valuation.balance - assert_equal 1400, most_recent_valuation.cash_balance - end - - # Updating "current balance" for a linked account is a pure system operation that manages the "current anchor" valuation - test "updating current balance for linked account modifies current anchor valuation" do - # TODO - end - - # A recon valuation is an override for a user to "reset" the balance from a specific date forward. - # This means, "The balance on X date is Y", which is then used as the new starting point to apply transactions against - test "manual accounts can add recon valuations at any point in the account timeline" do - assert_equal 1, @account.valuations.count - - @account.reconcile_balance!(balance: 1000, cash_balance: 1000, date: 2.days.ago.to_date) - - assert_equal 2, @account.valuations.count - - most_recent_valuation = @account.valuations.joins(:entry).order("entries.date DESC").first - - assert_equal 1000, most_recent_valuation.balance - assert_equal 1000, most_recent_valuation.cash_balance - end - - # While technically valid and possible to calculate, "recon" valuations for a linked account rarely make sense - # and add complexity. If the user has linked to a data provider, we expect the provider to be responsible for - # delivering the correct set of transactions to construct the historical balance - test "recon valuations are invalid for linked accounts" do - linked_account = accounts(:connected) - - assert_raises Account::InvalidBalanceError do - linked_account.reconcile_balance!(balance: 1000, cash_balance: 1000, date: 2.days.ago.to_date) - end - end - - test "sets or updates opening balance" do - Entry.destroy_all - - assert_equal 0, @account.entries.valuations.count - - # Creates non-existent opening valuation - @account.set_or_update_opening_balance!( - balance: 2000, - cash_balance: 2000, - date: 2.days.ago.to_date - ) - - opening_valuation_entry = @account.entries.first - - assert_equal 2000, opening_valuation_entry.amount - assert_equal 2.days.ago.to_date, opening_valuation_entry.date - assert_equal 2000, opening_valuation_entry.valuation.balance - assert_equal 2000, opening_valuation_entry.valuation.cash_balance - - # Updates existing opening valuation - @account.set_or_update_opening_balance!( - balance: 3000, - cash_balance: 3000 - ) - - opening_valuation_entry = @account.entries.first - - assert_equal 3000, opening_valuation_entry.amount - assert_equal 2.days.ago.to_date, opening_valuation_entry.date - assert_equal 3000, opening_valuation_entry.valuation.balance - assert_equal 3000, opening_valuation_entry.valuation.cash_balance - end - - # While we don't allow "recon" valuations for a linked account, we DO allow opening balance updates. This is because - # providers rarely give 100% of the transaction history (usually cuts off at 2 years), which can misrepresent the true - # "opening date" on the account and obscure longer net worth historical graphs. This is an *optional* way for the user - # to get their linked account histories "perfect". - test "can update the opening balance and date for a linked account" do - # TODO - end - - test "can update the opening balance and date for a manual account" do - # TODO - end end -- 2.53.0 From 25f0c78c47a5006b7021bebfd27b0f643edd0459 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 9 Jul 2025 22:28:07 -0400 Subject: [PATCH 10/12] Update properties controller to use new creational and update balance methods --- app/controllers/properties_controller.rb | 58 +++++--- .../controllers/money_field_controller.js | 29 ++++ app/models/account/overview_form.rb | 87 +++++++++++ app/models/account/reconcileable.rb | 66 ++++++--- app/models/family/account_creatable.rb | 50 ++++--- app/models/property.rb | 1 + app/views/properties/_form_tabs.html.erb | 2 +- .../_property_overview_fields.html.erb | 30 ++++ app/views/properties/balances.html.erb | 30 ---- app/views/properties/details.html.erb | 50 +++++++ app/views/properties/edit.html.erb | 2 +- app/views/properties/new.html.erb | 2 +- app/views/shared/_money_field.html.erb | 2 +- config/routes.rb | 4 +- .../controllers/properties_controller_test.rb | 88 ++++++----- test/models/account/overview_form_test.rb | 139 ++++++++++++++++++ test/models/account/reconcileable_test.rb | 4 +- 17 files changed, 500 insertions(+), 144 deletions(-) create mode 100644 app/models/account/overview_form.rb create mode 100644 app/views/properties/_property_overview_fields.html.erb delete mode 100644 app/views/properties/balances.html.erb create mode 100644 app/views/properties/details.html.erb create mode 100644 test/models/account/overview_form_test.rb diff --git a/app/controllers/properties_controller.rb b/app/controllers/properties_controller.rb index 8b2ec062..8ccdf718 100644 --- a/app/controllers/properties_controller.rb +++ b/app/controllers/properties_controller.rb @@ -1,31 +1,49 @@ class PropertiesController < ApplicationController include AccountableResource, StreamExtensions - before_action :set_property, only: [ :balances, :address, :update_balances, :update_address ] + before_action :set_property, only: [ :edit, :update, :details, :update_details, :address, :update_address ] def new @account = Current.family.accounts.build(accountable: Property.new) end def create - @account = Current.family.accounts.create!( - property_params.merge(currency: Current.family.currency, balance: 0, status: "draft") + @account = Current.family.create_property_account!( + name: property_params[:name], + current_value: property_params[:current_estimated_value].to_d, + purchase_price: property_params[:purchase_price].present? ? property_params[:purchase_price].to_d : nil, + purchase_date: property_params[:purchase_date], + currency: property_params[:currency] || Current.family.currency, + draft: true ) - redirect_to balances_property_path(@account) + redirect_to details_property_path(@account) end def update - if @account.update(property_params) + form = Account::OverviewForm.new( + account: @account, + name: property_params[:name], + currency: property_params[:currency], + opening_balance: property_params[:purchase_price], + opening_cash_balance: property_params[:purchase_price].present? ? "0" : nil, + opening_date: property_params[:purchase_date], + current_balance: property_params[:current_estimated_value], + current_cash_balance: property_params[:current_estimated_value].present? ? "0" : nil + ) + + result = form.save + + if result.success? @success_message = "Property details updated successfully." if @account.active? render :edit else - redirect_to balances_property_path(@account) + redirect_to details_property_path(@account) end else - @error_message = "Unable to update property details." + @error_message = result.error || "Unable to update property details." render :edit, status: :unprocessable_entity end end @@ -33,26 +51,25 @@ class PropertiesController < ApplicationController def edit end - def balances + def details end - def update_balances - result = @account.update_balance(balance: balance_params[:balance], currency: balance_params[:currency]) - - if result.success? - @success_message = result.updated? ? "Balance updated successfully." : "No changes made. Account is already up to date." + def update_details + if @account.update(details_params) + @success_message = "Property details updated successfully." if @account.active? - render :balances + render :details else redirect_to address_property_path(@account) end else - @error_message = result.error_message - render :balances, status: :unprocessable_entity + @error_message = "Unable to update property details." + render :details, status: :unprocessable_entity end end + def address @property = @account.property @property.address ||= Address.new @@ -78,8 +95,9 @@ class PropertiesController < ApplicationController end private - def balance_params - params.require(:account).permit(:balance, :currency) + def details_params + params.require(:account) + .permit(:subtype, accountable_attributes: [ :id, :year_built, :area_unit, :area_value ]) end def address_params @@ -89,7 +107,9 @@ class PropertiesController < ApplicationController def property_params params.require(:account) - .permit(:name, :subtype, :accountable_type, accountable_attributes: [ :id, :year_built, :area_unit, :area_value ]) + .permit(:name, :currency, :purchase_price, :purchase_date, :current_estimated_value, + :subtype, :accountable_type, + accountable_attributes: [ :id, :year_built, :area_unit, :area_value ]) end def set_property diff --git a/app/javascript/controllers/money_field_controller.js b/app/javascript/controllers/money_field_controller.js index 2aab2d16..f41ae874 100644 --- a/app/javascript/controllers/money_field_controller.js +++ b/app/javascript/controllers/money_field_controller.js @@ -5,10 +5,15 @@ import { CurrenciesService } from "services/currencies_service"; // when currency select change, update the input value with the correct placeholder and step export default class extends Controller { static targets = ["amount", "currency", "symbol"]; + static values = { syncCurrency: Boolean }; handleCurrencyChange(e) { const selectedCurrency = e.target.value; this.updateAmount(selectedCurrency); + + if (this.syncCurrencyValue) { + this.syncOtherMoneyFields(selectedCurrency); + } } updateAmount(currency) { @@ -24,4 +29,28 @@ export default class extends Controller { this.symbolTarget.innerText = currency.symbol; }); } + + syncOtherMoneyFields(selectedCurrency) { + // Find the form this money field belongs to + const form = this.element.closest("form"); + if (!form) return; + + // Find all other money field controllers in the same form + const allMoneyFields = form.querySelectorAll('[data-controller~="money-field"]'); + + allMoneyFields.forEach(field => { + // Skip the current field + if (field === this.element) return; + + // Get the controller instance + const controller = this.application.getControllerForElementAndIdentifier(field, "money-field"); + if (!controller) return; + + // Update the currency select if it exists + if (controller.hasCurrencyTarget) { + controller.currencyTarget.value = selectedCurrency; + controller.updateAmount(selectedCurrency); + } + }); + } } diff --git a/app/models/account/overview_form.rb b/app/models/account/overview_form.rb new file mode 100644 index 00000000..089711c8 --- /dev/null +++ b/app/models/account/overview_form.rb @@ -0,0 +1,87 @@ +class Account::OverviewForm + include ActiveModel::Model + + attr_accessor :account, :name, :currency, :opening_date + attr_reader :opening_balance, :opening_cash_balance, :current_balance, :current_cash_balance + + Result = Struct.new(:success?, :updated?, :error, keyword_init: true) + CurrencyUpdateError = Class.new(StandardError) + + def opening_balance=(value) + @opening_balance = value.nil? ? nil : value.to_d + end + + def opening_cash_balance=(value) + @opening_cash_balance = value.nil? ? nil : value.to_d + end + + def current_balance=(value) + @current_balance = value.nil? ? nil : value.to_d + end + + def current_cash_balance=(value) + @current_cash_balance = value.nil? ? nil : value.to_d + end + + def save + # Validate that balance fields are properly paired + if (!opening_balance.nil? && opening_cash_balance.nil?) || + (opening_balance.nil? && !opening_cash_balance.nil?) + raise ArgumentError, "Both opening_balance and opening_cash_balance must be provided together" + end + + if (!current_balance.nil? && current_cash_balance.nil?) || + (current_balance.nil? && !current_cash_balance.nil?) + raise ArgumentError, "Both current_balance and current_cash_balance must be provided together" + end + + updated = false + sync_required = false + + Account.transaction do + # Update name if provided + if name.present? && name != account.name + account.update!(name: name) + updated = true + end + + # Update currency if provided + if currency.present? && currency != account.currency + account.update_currency!(currency) + updated = true + sync_required = true + end + + # Update opening balance if provided (already validated that both are present) + if !opening_balance.nil? + account.set_or_update_opening_balance!( + balance: opening_balance, + cash_balance: opening_cash_balance, + date: opening_date # optional + ) + updated = true + sync_required = true + end + + # Update current balance if provided (already validated that both are present) + if !current_balance.nil? + account.update_current_balance!( + balance: current_balance, + cash_balance: current_cash_balance + ) + updated = true + sync_required = true + end + end + + # Only sync if transaction succeeded and sync is required + account.sync_later if sync_required + + Result.new(success?: true, updated?: updated) + rescue ArgumentError => e + # Re-raise ArgumentError as it's a developer error + raise e + rescue => e + Result.new(success?: false, updated?: false, error: e.message) + end +end diff --git a/app/models/account/reconcileable.rb b/app/models/account/reconcileable.rb index 6b6f1adc..dc6df605 100644 --- a/app/models/account/reconcileable.rb +++ b/app/models/account/reconcileable.rb @@ -17,39 +17,63 @@ module Account::Reconcileable balance - cash_balance end + def opening_balance + @opening_balance ||= opening_anchor_valuation&.balance + end + + def opening_cash_balance + @opening_cash_balance ||= opening_anchor_valuation&.cash_balance + end + + def opening_date + @opening_date ||= opening_anchor_valuation&.entry&.date + end + def reconcile_balance!(balance:, cash_balance:, date:) raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance > balance raise InvalidBalanceError, "Linked accounts cannot be reconciled" if linked? - existing_valuation = valuations.joins(:entry).where(kind: "recon", entry: { date: Date.current }).first + existing_valuation = valuations.joins(:entry).where(kind: "recon", entry: { date: date }).first - if existing_valuation.present? - existing_valuation.update!( - balance: balance, - cash_balance: cash_balance - ) - else - entries.create!( - date: date, - name: Valuation::Name.new("recon", self.accountable_type), - amount: balance, - currency: self.currency, - entryable: Valuation.new( - kind: "recon", + transaction do + if existing_valuation.present? + existing_valuation.update!( balance: balance, cash_balance: cash_balance ) - ) + else + entries.create!( + date: date, + name: Valuation::Name.new("recon", self.accountable_type), + amount: balance, + currency: self.currency, + entryable: Valuation.new( + kind: "recon", + balance: balance, + cash_balance: cash_balance + ) + ) + end + + # Update cached balance fields on account when reconciling for current date + if date == Date.current + update!(balance: balance, cash_balance: cash_balance) + end end end - def update_current_balance(balance:, cash_balance:) + def update_current_balance!(balance:, cash_balance:) raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance > balance - if opening_anchor_valuation.present? && valuations.where(kind: "recon").empty? - adjust_opening_balance_with_delta(balance:, cash_balance:) - else - reconcile_balance!(balance:, cash_balance:, date: Date.current) + transaction do + if opening_anchor_valuation.present? && valuations.where(kind: "recon").empty? + adjust_opening_balance_with_delta(balance:, cash_balance:) + else + reconcile_balance!(balance:, cash_balance:, date: Date.current) + end + + # Always update cached balance fields when updating current balance + update!(balance: balance, cash_balance: cash_balance) end end @@ -100,7 +124,7 @@ module Account::Reconcileable private def opening_anchor_valuation - valuations.opening_anchor.first + @opening_anchor_valuation ||= valuations.opening_anchor.includes(:entry).first end def current_anchor_valuation diff --git a/app/models/family/account_creatable.rb b/app/models/family/account_creatable.rb index 8ab6985c..9aa575d9 100644 --- a/app/models/family/account_creatable.rb +++ b/app/models/family/account_creatable.rb @@ -1,7 +1,7 @@ module Family::AccountCreatable extend ActiveSupport::Concern - def create_property_account!(name:, current_value:, purchase_price: nil, purchase_date: nil, currency: nil) + def create_property_account!(name:, current_value:, purchase_price: nil, purchase_date: nil, currency: nil, draft: false) create_manual_account!( name: name, balance: current_value, @@ -9,11 +9,12 @@ module Family::AccountCreatable accountable_type: Property, opening_balance: purchase_price, opening_date: purchase_date, - currency: currency + currency: currency, + draft: draft ) end - def create_vehicle_account!(name:, current_value:, purchase_price: nil, purchase_date: nil, currency: nil) + def create_vehicle_account!(name:, current_value:, purchase_price: nil, purchase_date: nil, currency: nil, draft: false) create_manual_account!( name: name, balance: current_value, @@ -21,23 +22,25 @@ module Family::AccountCreatable accountable_type: Vehicle, opening_balance: purchase_price, opening_date: purchase_date, - currency: currency + currency: currency, + draft: draft ) end - def create_depository_account!(name:, current_balance:, opening_date: nil, currency: nil) + def create_depository_account!(name:, current_balance:, opening_date: nil, currency: nil, draft: false) create_manual_account!( name: name, balance: current_balance, cash_balance: current_balance, accountable_type: Depository, opening_date: opening_date, - currency: currency + currency: currency, + draft: draft ) end # Investment account values are built up by adding holdings / trades, not by initializing a "balance" - def create_investment_account!(name:, currency: nil) + def create_investment_account!(name:, currency: nil, draft: false) create_manual_account!( name: name, balance: 0, @@ -45,11 +48,12 @@ module Family::AccountCreatable accountable_type: Investment, opening_balance: 0, # Investment accounts start empty opening_cash_balance: 0, - currency: currency + currency: currency, + draft: draft ) end - def create_other_asset_account!(name:, current_value:, purchase_price: nil, purchase_date: nil, currency: nil) + def create_other_asset_account!(name:, current_value:, purchase_price: nil, purchase_date: nil, currency: nil, draft: false) create_manual_account!( name: name, balance: current_value, @@ -57,11 +61,12 @@ module Family::AccountCreatable accountable_type: OtherAsset, opening_balance: purchase_price, opening_date: purchase_date, - currency: currency + currency: currency, + draft: draft ) end - def create_other_liability_account!(name:, current_debt:, original_debt: nil, origination_date: nil, currency: nil) + def create_other_liability_account!(name:, current_debt:, original_debt: nil, origination_date: nil, currency: nil, draft: false) create_manual_account!( name: name, balance: current_debt, @@ -69,12 +74,13 @@ module Family::AccountCreatable accountable_type: OtherLiability, opening_balance: original_debt, opening_date: origination_date, - currency: currency + currency: currency, + draft: draft ) end # For now, crypto accounts are very simple; we just track overall value - def create_crypto_account!(name:, current_value:, currency: nil) + def create_crypto_account!(name:, current_value:, currency: nil, draft: false) create_manual_account!( name: name, balance: current_value, @@ -82,11 +88,12 @@ module Family::AccountCreatable accountable_type: Crypto, opening_balance: current_value, opening_cash_balance: current_value, - currency: currency + currency: currency, + draft: draft ) end - def create_credit_card_account!(name:, current_debt:, opening_date: nil, currency: nil) + def create_credit_card_account!(name:, current_debt:, opening_date: nil, currency: nil, draft: false) create_manual_account!( name: name, balance: current_debt, @@ -94,11 +101,12 @@ module Family::AccountCreatable accountable_type: CreditCard, opening_balance: 0, # Credit cards typically start with no debt opening_date: opening_date, - currency: currency + currency: currency, + draft: draft ) end - def create_loan_account!(name:, current_principal:, original_principal: nil, origination_date: nil, currency: nil) + def create_loan_account!(name:, current_principal:, original_principal: nil, origination_date: nil, currency: nil, draft: false) create_manual_account!( name: name, balance: current_principal, @@ -106,7 +114,8 @@ module Family::AccountCreatable accountable_type: Loan, opening_balance: original_principal, opening_date: origination_date, - currency: currency + currency: currency, + draft: draft ) end @@ -128,14 +137,15 @@ module Family::AccountCreatable private - def create_manual_account!(name:, balance:, cash_balance:, accountable_type:, opening_balance: nil, opening_cash_balance: nil, opening_date: nil, currency: nil) + def create_manual_account!(name:, balance:, cash_balance:, accountable_type:, opening_balance: nil, opening_cash_balance: nil, opening_date: nil, currency: nil, draft: false) Family.transaction do account = accounts.create!( name: name, balance: balance, cash_balance: cash_balance, currency: currency.presence || self.currency, - accountable: accountable_type.new + accountable: accountable_type.new, + status: draft ? "draft" : "active" ) account.set_or_update_opening_balance!( diff --git a/app/models/property.rb b/app/models/property.rb index 6114a9f4..4868e42d 100644 --- a/app/models/property.rb +++ b/app/models/property.rb @@ -52,6 +52,7 @@ class Property < ApplicationRecord private def first_valuation_amount + return nil unless account account.entries.valuations.order(:date).first&.amount_money || account.balance_money end end diff --git a/app/views/properties/_form_tabs.html.erb b/app/views/properties/_form_tabs.html.erb index 0693c7c9..9e8b2e6b 100644 --- a/app/views/properties/_form_tabs.html.erb +++ b/app/views/properties/_form_tabs.html.erb @@ -2,6 +2,6 @@
<%= render "properties/form_tab", label: "Overview", href: account.new_record? ? nil : edit_property_path(@account), active: active_tab == "overview" %> - <%= render "properties/form_tab", label: "Value", href: account.new_record? ? nil : balances_property_path(@account), active: active_tab == "value" %> + <%= render "properties/form_tab", label: "Details", href: account.new_record? ? nil : details_property_path(@account), active: active_tab == "details" %> <%= render "properties/form_tab", label: "Address", href: account.new_record? ? nil : address_property_path(@account), active: active_tab == "address" %>
diff --git a/app/views/properties/_property_overview_fields.html.erb b/app/views/properties/_property_overview_fields.html.erb new file mode 100644 index 00000000..401f618b --- /dev/null +++ b/app/views/properties/_property_overview_fields.html.erb @@ -0,0 +1,30 @@ +<%# locals: (form:, account:) %> + +
+ <%= form.text_field :name, + label: "Name", + placeholder: "Vacation home", + required: true %> + + <%= form.money_field :current_estimated_value, + label: "Current estimated value", + label_tooltip: "The estimated market value of your property. This number can often be found on sites like Zillow or Redfin, and is never an exact number.", + placeholder: Money.new(0, form.object.currency || Current.family.currency), + value: account.balance, + required: true, + sync_currency: true %> + + <%= form.money_field :purchase_price, + label: "Purchase price", + label_tooltip: "The amount you paid when you purchased the property. Leave blank if unknown.", + placeholder: Money.new(0, form.object.currency || Current.family.currency), + value: account.opening_balance, + sync_currency: true %> + + <%= form.date_field :purchase_date, + label: "Purchase date", + label_tooltip: "The date you purchased the property. This helps track your property's value over time.", + value: account.opening_date %> + + <%= form.hidden_field :current_cash_balance, value: 0 %> +
diff --git a/app/views/properties/balances.html.erb b/app/views/properties/balances.html.erb deleted file mode 100644 index c9141771..00000000 --- a/app/views/properties/balances.html.erb +++ /dev/null @@ -1,30 +0,0 @@ -<%= render DialogComponent.new do |dialog| %> - <% dialog.with_header(title: "Enter property manually") %> - <% dialog.with_body do %> -
- <%= render "properties/form_tabs", account: @account, active_tab: "value" %> - - -
- <%= styled_form_with model: @account, url: update_balances_property_path(@account), method: :patch do |form| %> -
- <%= render "properties/form_alert", notice: @success_message, error: @error_message %> - - <%= form.money_field :balance, - label: "Estimated market value", - label_tooltip: "The estimated market value of your property. This number can often be found on sites like Zillow or Redfin, and is never an exact number.", - placeholder: "0" %> -
- - -
- <%= render ButtonComponent.new( - text: @account.active? ? "Save" : "Next", - variant: "primary", - ) %> -
- <% end %> -
-
- <% end %> -<% end %> diff --git a/app/views/properties/details.html.erb b/app/views/properties/details.html.erb new file mode 100644 index 00000000..65a6a009 --- /dev/null +++ b/app/views/properties/details.html.erb @@ -0,0 +1,50 @@ +<%= render DialogComponent.new do |dialog| %> + <% dialog.with_header(title: "Enter property manually") %> + <% dialog.with_body do %> +
+ + <%= render "properties/form_tabs", account: @account, active_tab: "details" %> + + +
+ <%= styled_form_with model: @account, url: update_details_property_path(@account), method: :patch do |form| %> +
+ <%= render "properties/form_alert", notice: @success_message, error: @error_message %> + + <%= form.select :subtype, + options_for_select(Property::SUBTYPES.map { |k, v| [v[:long], k] }, @account.subtype), + { label: "Property type" }, + { class: "form-field__input" } %> + + <%= form.fields_for :accountable do |property_form| %> +
+ <%= property_form.number_field :area_value, + label: "Area", + placeholder: "1200", + min: 0 %> + <%= property_form.select :area_unit, + [["Square Feet", "sqft"], ["Square Meters", "sqm"]], + { label: "Area unit", selected: @account.accountable.area_unit } %> +
+ + <%= property_form.number_field :year_built, + label: "Year built", + placeholder: "2000", + step: 1, + min: 1800, + max: Date.current.year %> + <% end %> +
+ + +
+ <%= render ButtonComponent.new( + text: @account.active? ? "Save" : "Next", + variant: "primary", + ) %> +
+ <% end %> +
+
+ <% end %> +<% end %> diff --git a/app/views/properties/edit.html.erb b/app/views/properties/edit.html.erb index fd75d929..7e5de7ac 100644 --- a/app/views/properties/edit.html.erb +++ b/app/views/properties/edit.html.erb @@ -10,7 +10,7 @@ <%= styled_form_with model: @account, url: property_path(@account), method: :patch do |form| %>
<%= render "properties/form_alert", notice: @success_message, error: @error_message %> - <%= render "properties/overview_fields", form: form %> + <%= render "properties/property_overview_fields", form: form, account: @account %>
diff --git a/app/views/properties/new.html.erb b/app/views/properties/new.html.erb index 73dae294..ee1e08c9 100644 --- a/app/views/properties/new.html.erb +++ b/app/views/properties/new.html.erb @@ -10,7 +10,7 @@ <%= styled_form_with model: @account, url: properties_path do |form| %>
<%= render "properties/form_alert", notice: @success_message, error: @error_message %> - <%= render "properties/overview_fields", form: form %> + <%= render "properties/property_overview_fields", form: form, account: @account %>
diff --git a/app/views/shared/_money_field.html.erb b/app/views/shared/_money_field.html.erb index 05929401..59449936 100644 --- a/app/views/shared/_money_field.html.erb +++ b/app/views/shared/_money_field.html.erb @@ -7,7 +7,7 @@ end currency = Money::Currency.new(currency_value || options[:default_currency] || "USD") %> -
+
> <% if options[:label_tooltip] %>
<%= form.label options[:label] || t(".label"), class: "form-field__label" do %> diff --git a/config/routes.rb b/config/routes.rb index f0414287..09ecc211 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -170,8 +170,8 @@ Rails.application.routes.draw do resources :investments, except: :index resources :properties, except: :index do member do - get :balances - patch :update_balances + get :details + patch :update_details get :address patch :update_address diff --git a/test/controllers/properties_controller_test.rb b/test/controllers/properties_controller_test.rb index b5f1305f..9295a0bd 100644 --- a/test/controllers/properties_controller_test.rb +++ b/test/controllers/properties_controller_test.rb @@ -8,18 +8,15 @@ class PropertiesControllerTest < ActionDispatch::IntegrationTest @account = accounts(:property) end - test "creates property in draft status and redirects to balances step" do + test "creates property in draft status with initial balance information and redirects to details step" do assert_difference -> { Account.count } => 1 do post properties_path, params: { account: { name: "New Property", - subtype: "house", - accountable_type: "Property", - accountable_attributes: { - year_built: 1990, - area_value: 1200, - area_unit: "sqft" - } + purchase_price: "250000", + purchase_date: "2023-01-01", + current_estimated_value: "300000", + currency: "USD" } } end @@ -27,38 +24,47 @@ class PropertiesControllerTest < ActionDispatch::IntegrationTest created_account = Account.order(:created_at).last assert created_account.accountable.is_a?(Property) assert_equal "draft", created_account.status - assert_equal 0, created_account.balance - assert_equal 1990, created_account.accountable.year_built - assert_equal 1200, created_account.accountable.area_value - assert_equal "sqft", created_account.accountable.area_unit - assert_redirected_to balances_property_path(created_account) + assert_equal "New Property", created_account.name + assert_equal 300_000, created_account.balance + assert_equal 0, created_account.cash_balance + assert_equal "USD", created_account.currency + + # Check opening balance was set + opening_valuation = created_account.valuations.opening_anchor.first + assert_not_nil opening_valuation + assert_equal 250_000, opening_valuation.balance + assert_equal Date.parse("2023-01-01"), opening_valuation.entry.date + + assert_redirected_to details_property_path(created_account) end - test "updates property overview" do + test "updates property overview with balance information" do assert_no_difference [ "Account.count", "Property.count" ] do patch property_path(@account), params: { account: { name: "Updated Property", - subtype: "condo" + current_estimated_value: "350000", + currency: "USD" } } end @account.reload assert_equal "Updated Property", @account.name - assert_equal "condo", @account.subtype + assert_equal 350_000, @account.balance + assert_equal 0, @account.cash_balance - # If account is active, it renders edit view; otherwise redirects to balances + # If account is active, it renders edit view; otherwise redirects to details if @account.active? assert_response :success else - assert_redirected_to balances_property_path(@account) + assert_redirected_to details_property_path(@account) end end # Tab view tests - test "shows balances tab" do - get balances_property_path(@account) + test "shows details tab" do + get details_property_path(@account) assert_response :success end @@ -68,22 +74,26 @@ class PropertiesControllerTest < ActionDispatch::IntegrationTest end # Tab update tests - test "updates balances tab" do - original_balance = @account.balance - - # Mock the update_balance method to return a successful result - Account::BalanceUpdater::Result.any_instance.stubs(:success?).returns(true) - Account::BalanceUpdater::Result.any_instance.stubs(:updated?).returns(true) - - patch update_balances_property_path(@account), params: { + test "updates property details" do + patch update_details_property_path(@account), params: { account: { - balance: 600000, - currency: "EUR" + subtype: "condo", + accountable_attributes: { + year_built: 2005, + area_value: 1500, + area_unit: "sqft" + } } } - # If account is active, it renders balances view; otherwise redirects to address - if @account.reload.active? + @account.reload + assert_equal "condo", @account.subtype + assert_equal 2005, @account.accountable.year_built + assert_equal 1500, @account.accountable.area_value + assert_equal "sqft", @account.accountable.area_unit + + # If account is active, it renders details view; otherwise redirects to address + if @account.active? assert_response :success else assert_redirected_to address_property_path(@account) @@ -115,20 +125,6 @@ class PropertiesControllerTest < ActionDispatch::IntegrationTest end end - test "balances update handles validation errors" do - # Mock update_balance to return a failure result - Account::BalanceUpdater::Result.any_instance.stubs(:success?).returns(false) - Account::BalanceUpdater::Result.any_instance.stubs(:error_message).returns("Invalid balance") - - patch update_balances_property_path(@account), params: { - account: { - balance: 600000, - currency: "EUR" - } - } - - assert_response :unprocessable_entity - end test "address update handles validation errors" do Property.any_instance.stubs(:update).returns(false) diff --git a/test/models/account/overview_form_test.rb b/test/models/account/overview_form_test.rb new file mode 100644 index 00000000..351cf682 --- /dev/null +++ b/test/models/account/overview_form_test.rb @@ -0,0 +1,139 @@ +require "test_helper" + +class Account::OverviewFormTest < ActiveSupport::TestCase + setup do + @account = accounts(:property) + end + + test "initializes with account and attributes" do + form = Account::OverviewForm.new( + account: @account, + name: "Updated Property" + ) + + assert_equal @account, form.account + assert_equal "Updated Property", form.name + end + + test "save returns result with success and updated status" do + form = Account::OverviewForm.new(account: @account) + result = form.save + + assert result.success? + assert_not result.updated? + end + + test "updates account name when provided" do + form = Account::OverviewForm.new( + account: @account, + name: "New Property Name" + ) + + @account.expects(:update!).with(name: "New Property Name").once + @account.expects(:sync_later).never # Name change should not trigger sync + + result = form.save + + assert result.success? + assert result.updated? + end + + test "updates currency and triggers sync" do + form = Account::OverviewForm.new( + account: @account, + currency: "EUR" + ) + + @account.expects(:update_currency!).with("EUR").once + @account.expects(:sync_later).once # Currency change should trigger sync + + result = form.save + + assert result.success? + assert result.updated? + end + + test "calls sync_later only once for multiple balance-related changes" do + form = Account::OverviewForm.new( + account: @account, + currency: "EUR", + opening_balance: 100_000, + opening_cash_balance: 0, + current_balance: 150_000, + current_cash_balance: 0 + ) + + @account.expects(:update_currency!).with("EUR").once + @account.expects(:set_or_update_opening_balance!).once + @account.expects(:update_current_balance!).once + @account.expects(:sync_later).once # Should only be called once despite multiple changes + + result = form.save + + assert result.success? + assert result.updated? + end + + test "does not call sync_later when transaction fails" do + form = Account::OverviewForm.new( + account: @account, + name: "New Name", + opening_balance: 100_000, + opening_cash_balance: 0 + ) + + # Simulate a validation error on opening balance update + @account.expects(:update!).with(name: "New Name").once + @account.expects(:set_or_update_opening_balance!).raises(Account::Reconcileable::InvalidBalanceError.new("Cash balance cannot exceed balance")) + @account.expects(:sync_later).never # Should NOT sync if any update fails + + result = form.save + + assert_not result.success? + assert_not result.updated? + assert_equal "Cash balance cannot exceed balance", result.error + end + + test "raises ArgumentError when balance fields are not properly paired" do + # Opening balance without cash balance + form = Account::OverviewForm.new( + account: @account, + opening_balance: 100_000 + ) + + # Debug what values we have + assert_equal 100_000.to_d, form.opening_balance + assert_nil form.opening_cash_balance + + error = assert_raises(ArgumentError) do + form.save + end + assert_equal "Both opening_balance and opening_cash_balance must be provided together", error.message + + # Current cash balance without balance + form = Account::OverviewForm.new( + account: @account, + current_cash_balance: 0 + ) + + error = assert_raises(ArgumentError) do + form.save + end + assert_equal "Both current_balance and current_cash_balance must be provided together", error.message + end + + test "converts string balance values to decimals" do + form = Account::OverviewForm.new( + account: @account, + opening_balance: "100000.50", + opening_cash_balance: "0", + current_balance: "150000.75", + current_cash_balance: "5000.25" + ) + + assert_equal 100000.50.to_d, form.opening_balance + assert_equal 0.to_d, form.opening_cash_balance + assert_equal 150000.75.to_d, form.current_balance + assert_equal 5000.25.to_d, form.current_cash_balance + end +end diff --git a/test/models/account/reconcileable_test.rb b/test/models/account/reconcileable_test.rb index 334e907d..39fa7ade 100644 --- a/test/models/account/reconcileable_test.rb +++ b/test/models/account/reconcileable_test.rb @@ -64,7 +64,7 @@ class Account::ReconcileableTest < ActiveSupport::TestCase # No new valuation is appended; we're just adjusting the opening valuation anchor assert_no_difference "account.entries.count" do - account.update_current_balance(balance: 1000, cash_balance: 1000) + account.update_current_balance!(balance: 1000, cash_balance: 1000) end opening_valuation = account.valuations.first @@ -113,7 +113,7 @@ class Account::ReconcileableTest < ActiveSupport::TestCase assert_equal 2, account.valuations.count # Here, we assume user is once again "overriding" the balance to 1400 - account.update_current_balance(balance: 1400, cash_balance: 1400) + account.update_current_balance!(balance: 1400, cash_balance: 1400) most_recent_valuation = account.valuations.joins(:entry).order("entries.date DESC").first -- 2.53.0 From d80cb9f8128451b6cbcf0a32d33871c5ffba8d87 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Thu, 10 Jul 2025 10:31:40 -0400 Subject: [PATCH 11/12] Migrate valuations controller to new reconciliation methods --- app/components/button_component.html.erb | 2 +- app/components/buttonish_component.rb | 2 +- .../concerns/accountable_resource.rb | 37 ++++----- app/controllers/credit_cards_controller.rb | 27 +++++++ app/controllers/loans_controller.rb | 27 +++++++ app/controllers/valuations_controller.rb | 76 +++++++++---------- app/models/account.rb | 8 -- app/models/account/balance_updater.rb | 55 -------------- app/models/account/overview_form.rb | 1 + app/models/account/reconcileable.rb | 63 ++++++++++----- app/views/accounts/show/_activity.html.erb | 2 +- app/views/valuations/_form.html.erb | 4 +- app/views/valuations/new.html.erb | 8 +- .../controllers/valuations_controller_test.rb | 2 +- .../entryable_resource_interface_test.rb | 2 +- test/models/account/overview_form_test.rb | 2 + test/models/account/reconcileable_test.rb | 13 +--- test/models/account_test.rb | 9 +++ test/system/accounts_test.rb | 18 +++-- 19 files changed, 187 insertions(+), 171 deletions(-) delete mode 100644 app/models/account/balance_updater.rb diff --git a/app/components/button_component.html.erb b/app/components/button_component.html.erb index 75f3853e..0b69c464 100644 --- a/app/components/button_component.html.erb +++ b/app/components/button_component.html.erb @@ -1,6 +1,6 @@ <%= container do %> <% if icon && (icon_position != :right) %> - <%= helpers.icon(icon, size: size, color: icon_color) %> + <%= helpers.icon(icon, size: size, color: icon_color, class: icon_classes) %> <% end %> <% unless icon_only? %> diff --git a/app/components/buttonish_component.rb b/app/components/buttonish_component.rb index 89e3afda..4bbb2882 100644 --- a/app/components/buttonish_component.rb +++ b/app/components/buttonish_component.rb @@ -5,7 +5,7 @@ class ButtonishComponent < ViewComponent::Base icon_classes: "fg-inverse" }, secondary: { - container_classes: "text-secondary bg-gray-50 theme-dark:bg-gray-700 hover:bg-gray-100 theme-dark:hover:bg-gray-600 disabled:bg-gray-200 theme-dark:disabled:bg-gray-600", + container_classes: "text-primary bg-gray-50 theme-dark:bg-gray-700 hover:bg-gray-100 theme-dark:hover:bg-gray-600 disabled:bg-gray-200 theme-dark:disabled:bg-gray-600", icon_classes: "fg-primary" }, destructive: { diff --git a/app/controllers/concerns/accountable_resource.rb b/app/controllers/concerns/accountable_resource.rb index bfdc465f..445a7335 100644 --- a/app/controllers/concerns/accountable_resource.rb +++ b/app/controllers/concerns/accountable_resource.rb @@ -46,29 +46,24 @@ module AccountableResource end def update - # Handle balance update if provided - if account_params[:balance].present? - result = @account.update_balance(balance: account_params[:balance], currency: account_params[:currency]) - unless result.success? - @error_message = result.error_message - render :edit, status: :unprocessable_entity - return + form = Account::OverviewForm.new( + account: @account, + name: account_params[:name], + currency: account_params[:currency], + current_balance: account_params[:balance], + current_cash_balance: @account.depository? ? account_params[:balance] : "0" + ) + + result = form.save + + if result.success? + respond_to do |format| + format.html { redirect_back_or_to account_path(@account), notice: accountable_type.name.underscore.humanize + " account updated" } + format.turbo_stream { stream_redirect_to account_path(@account), notice: accountable_type.name.underscore.humanize + " account updated" } end - end - - # Update remaining account attributes - update_params = account_params.except(:return_to, :balance, :currency, :tracking_start_date) - unless @account.update(update_params) - @error_message = @account.errors.full_messages.join(", ") + else + @error_message = result.error || "Unable to update account details." render :edit, status: :unprocessable_entity - return - end - - @account.lock_saved_attributes! - - respond_to do |format| - format.html { redirect_back_or_to account_path(@account), notice: accountable_type.name.underscore.humanize + " account updated" } - format.turbo_stream { stream_redirect_to account_path(@account), notice: accountable_type.name.underscore.humanize + " account updated" } end end diff --git a/app/controllers/credit_cards_controller.rb b/app/controllers/credit_cards_controller.rb index 4d0f5659..5cf8cd62 100644 --- a/app/controllers/credit_cards_controller.rb +++ b/app/controllers/credit_cards_controller.rb @@ -9,4 +9,31 @@ class CreditCardsController < ApplicationController :annual_fee, :expiration_date ) + + def update + form = Account::OverviewForm.new( + account: @account, + name: account_params[:name], + currency: account_params[:currency], + current_balance: account_params[:balance], + current_cash_balance: @account.depository? ? account_params[:balance] : "0" + ) + + result = form.save + + if result.success? + # Update credit card-specific attributes + if account_params[:accountable_attributes].present? + @account.credit_card.update!(account_params[:accountable_attributes]) + end + + respond_to do |format| + format.html { redirect_back_or_to account_path(@account), notice: "Credit card account updated" } + format.turbo_stream { stream_redirect_to account_path(@account), notice: "Credit card account updated" } + end + else + @error_message = result.error || "Unable to update account details." + render :edit, status: :unprocessable_entity + end + end end diff --git a/app/controllers/loans_controller.rb b/app/controllers/loans_controller.rb index 961c5acf..c6f80835 100644 --- a/app/controllers/loans_controller.rb +++ b/app/controllers/loans_controller.rb @@ -4,4 +4,31 @@ class LoansController < ApplicationController permitted_accountable_attributes( :id, :rate_type, :interest_rate, :term_months, :initial_balance ) + + def update + form = Account::OverviewForm.new( + account: @account, + name: account_params[:name], + currency: account_params[:currency], + current_balance: account_params[:balance], + current_cash_balance: @account.depository? ? account_params[:balance] : "0" + ) + + result = form.save + + if result.success? + # Update loan-specific attributes + if account_params[:accountable_attributes].present? + @account.loan.update!(account_params[:accountable_attributes]) + end + + respond_to do |format| + format.html { redirect_back_or_to account_path(@account), notice: "Loan account updated" } + format.turbo_stream { stream_redirect_to account_path(@account), notice: "Loan account updated" } + end + else + @error_message = result.error || "Unable to update account details." + render :edit, status: :unprocessable_entity + end + end end diff --git a/app/controllers/valuations_controller.rb b/app/controllers/valuations_controller.rb index 90aa1da0..965d26ec 100644 --- a/app/controllers/valuations_controller.rb +++ b/app/controllers/valuations_controller.rb @@ -4,59 +4,55 @@ class ValuationsController < ApplicationController def create account = Current.family.accounts.find(params.dig(:entry, :account_id)) - result = account.update_balance( - balance: entry_params[:amount], - date: entry_params[:date], - currency: entry_params[:currency], - notes: entry_params[:notes] - ) - - if result.success? - @success_message = result.updated? ? "Balance updated" : "No changes made. Account is already up to date." - - respond_to do |format| - format.html { redirect_back_or_to account_path(account), notice: @success_message } - format.turbo_stream { stream_redirect_back_or_to(account_path(account), notice: @success_message) } - end + if entry_params[:date].to_date == Date.current + account.update_current_balance!(balance: entry_params[:amount].to_d) else - @error_message = result.error_message - render :new, status: :unprocessable_entity + account.reconcile_balance!( + balance: entry_params[:amount].to_d, + date: entry_params[:date].to_date + ) + end + + account.sync_later + + respond_to do |format| + format.html { redirect_back_or_to account_path(account), notice: "Account value updated" } + format.turbo_stream { stream_redirect_back_or_to(account_path(account), notice: "Account value updated") } end end def update - result = @entry.account.update_balance( - date: @entry.date, - balance: entry_params[:amount], - currency: entry_params[:currency], - notes: entry_params[:notes] + # ActiveRecord::Base.transaction do + @entry.account.reconcile_balance!( + balance: entry_params[:amount].to_d, + date: entry_params[:date].to_date ) - if result.success? - @entry.reload + if entry_params[:notes].present? + @entry.update!(notes: entry_params[:notes]) + end - respond_to do |format| - format.html { redirect_back_or_to account_path(@entry.account), notice: result.updated? ? "Balance updated" : "No changes made. Account is already up to date." } - format.turbo_stream do - render turbo_stream: [ - turbo_stream.replace( - dom_id(@entry, :header), - partial: "valuations/header", - locals: { entry: @entry } - ), - turbo_stream.replace(@entry) - ] - end + @entry.account.sync_later + + @entry.reload + + respond_to do |format| + format.html { redirect_back_or_to account_path(@entry.account), notice: "Account value updated" } + format.turbo_stream do + render turbo_stream: [ + turbo_stream.replace( + dom_id(@entry, :header), + partial: "valuations/header", + locals: { entry: @entry } + ), + turbo_stream.replace(@entry) + ] end - else - @error_message = result.error_message - render :show, status: :unprocessable_entity end end private def entry_params - params.require(:entry) - .permit(:date, :amount, :currency, :notes) + params.require(:entry).permit(:date, :amount, :notes) end end diff --git a/app/models/account.rb b/app/models/account.rb index c7cbfd53..e45ff52a 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -14,8 +14,6 @@ class Account < ApplicationRecord has_many :holdings, dependent: :destroy has_many :balances, dependent: :destroy - - enum :classification, { asset: "asset", liability: "liability" }, validate: { allow_nil: true } scope :visible, -> { where(status: [ "draft", "active" ]) } @@ -120,11 +118,6 @@ class Account < ApplicationRecord .order(amount: :desc) end - - def update_balance(balance:, date: Date.current, currency: nil, notes: nil) - Account::BalanceUpdater.new(self, balance:, currency:, date:, notes:).update - end - def update_currency!(new_currency) raise "Currency cannot be changed" if linked? @@ -134,7 +127,6 @@ class Account < ApplicationRecord end end - def start_date first_entry_date = entries.minimum(:date) || Date.current first_entry_date - 1.day diff --git a/app/models/account/balance_updater.rb b/app/models/account/balance_updater.rb deleted file mode 100644 index d1e9f01e..00000000 --- a/app/models/account/balance_updater.rb +++ /dev/null @@ -1,55 +0,0 @@ -class Account::BalanceUpdater - def initialize(account, balance:, currency: nil, date: Date.current, notes: nil) - @account = account - @balance = balance.to_d - @currency = currency - @date = date.to_date - @notes = notes - end - - def update - return Result.new(success?: true, updated?: false) unless requires_update? - - Account.transaction do - if date == Date.current - account.balance = balance - account.currency = currency if currency.present? - account.save! - end - - valuation_entry = account.entries.valuations.find_or_initialize_by(date: date) do |entry| - entry.entryable = Valuation.new( - kind: "recon", - balance: balance, - cash_balance: balance - ) - end - - valuation_entry.amount = balance - valuation_entry.currency = currency if currency.present? - valuation_entry.name = valuation_name(valuation_entry, account) - valuation_entry.notes = notes if notes.present? - valuation_entry.save! - end - - account.sync_later - - Result.new(success?: true, updated?: true) - rescue => e - message = Rails.env.development? ? e.message : "Unable to update account values. Please try again." - Result.new(success?: false, updated?: false, error_message: message) - end - - private - attr_reader :account, :balance, :currency, :date, :notes - - Result = Struct.new(:success?, :updated?, :error_message) - - def requires_update? - date != Date.current || account.balance != balance || account.currency != currency - end - - def valuation_name(valuation_entry, account) - Valuation::Name.new(valuation_entry.entryable.kind, account.accountable_type).to_s - end -end diff --git a/app/models/account/overview_form.rb b/app/models/account/overview_form.rb index 089711c8..38313e41 100644 --- a/app/models/account/overview_form.rb +++ b/app/models/account/overview_form.rb @@ -42,6 +42,7 @@ class Account::OverviewForm # Update name if provided if name.present? && name != account.name account.update!(name: name) + account.lock_attr!(:name) updated = true end diff --git a/app/models/account/reconcileable.rb b/app/models/account/reconcileable.rb index dc6df605..3d9f19c9 100644 --- a/app/models/account/reconcileable.rb +++ b/app/models/account/reconcileable.rb @@ -29,18 +29,26 @@ module Account::Reconcileable @opening_date ||= opening_anchor_valuation&.entry&.date end - def reconcile_balance!(balance:, cash_balance:, date:) - raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance > balance + def reconcile_balance!(balance:, cash_balance: nil, date: nil) + raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance.present? && cash_balance > balance raise InvalidBalanceError, "Linked accounts cannot be reconciled" if linked? + derived_cash_balance = cash_balance.present? ? cash_balance : choose_cash_balance_from_balance(balance) + + if date.nil? + update_current_balance!(balance:, cash_balance: derived_cash_balance) + return + end + existing_valuation = valuations.joins(:entry).where(kind: "recon", entry: { date: date }).first transaction do if existing_valuation.present? existing_valuation.update!( balance: balance, - cash_balance: cash_balance + cash_balance: derived_cash_balance ) + existing_valuation.entry.update!(amount: balance) else entries.create!( date: date, @@ -50,43 +58,36 @@ module Account::Reconcileable entryable: Valuation.new( kind: "recon", balance: balance, - cash_balance: cash_balance + cash_balance: derived_cash_balance ) ) end # Update cached balance fields on account when reconciling for current date if date == Date.current - update!(balance: balance, cash_balance: cash_balance) + update!(balance: balance, cash_balance: derived_cash_balance) end end end - def update_current_balance!(balance:, cash_balance:) - raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance > balance + def update_current_balance!(balance:, cash_balance: nil) + raise InvalidBalanceError, "Cash balance cannot exceed balance" if cash_balance.present? && cash_balance > balance + + derived_cash_balance = cash_balance.present? ? cash_balance : choose_cash_balance_from_balance(balance) transaction do - if opening_anchor_valuation.present? && valuations.where(kind: "recon").empty? - adjust_opening_balance_with_delta(balance:, cash_balance:) + # See test for explanation - Depository accounts are handled as a special case for current balance updates + if opening_anchor_valuation.present? && valuations.where(kind: "recon").empty? && self.depository? + adjust_opening_balance_with_delta!(balance:, cash_balance: derived_cash_balance) else - reconcile_balance!(balance:, cash_balance:, date: Date.current) + reconcile_balance!(balance:, cash_balance: derived_cash_balance, date: Date.current) end # Always update cached balance fields when updating current balance - update!(balance: balance, cash_balance: cash_balance) + update!(balance: balance, cash_balance: derived_cash_balance) end end - def adjust_opening_balance_with_delta(balance:, cash_balance:) - delta = self.balance - balance - cash_delta = self.cash_balance - cash_balance - - set_or_update_opening_balance!( - balance: balance - delta, - cash_balance: cash_balance - cash_delta - ) - end - def set_or_update_opening_balance!(balance:, cash_balance:, date: nil) # A reasonable start date for most accounts to fill up adequate history for graphs fallback_opening_date = 2.years.ago.to_date @@ -130,4 +131,24 @@ module Account::Reconcileable def current_anchor_valuation valuations.current_anchor.first end + + def adjust_opening_balance_with_delta!(balance:, cash_balance:) + delta = self.balance - balance + cash_delta = self.cash_balance - cash_balance + + set_or_update_opening_balance!( + balance: balance - delta, + cash_balance: cash_balance - cash_delta + ) + end + + # For depository accounts, the cash balance is the same as the balance always + # Otherwise, if not specified, we assume cash balance is 0 + def choose_cash_balance_from_balance(balance) + if self.depository? + balance + else + 0 + end + end end diff --git a/app/views/accounts/show/_activity.html.erb b/app/views/accounts/show/_activity.html.erb index ab65dd4c..ebbdcb5c 100644 --- a/app/views/accounts/show/_activity.html.erb +++ b/app/views/accounts/show/_activity.html.erb @@ -10,7 +10,7 @@ <% menu.with_item( variant: "link", - text: "New balance", + text: "Account #{account.asset? ? "value" : "balance"} update", icon: "circle-dollar-sign", href: new_valuation_path(account_id: @account.id), data: { turbo_frame: :modal }) %> diff --git a/app/views/valuations/_form.html.erb b/app/views/valuations/_form.html.erb index 5429f3a7..cd037eb0 100644 --- a/app/views/valuations/_form.html.erb +++ b/app/views/valuations/_form.html.erb @@ -10,8 +10,8 @@
<%= form.hidden_field :name, value: "Balance update" %> <%= form.date_field :date, label: true, required: true, value: Date.current, min: Entry.min_supported_date, max: Date.current %> - <%= form.money_field :amount, label: t(".amount"), required: true %> + <%= form.money_field :amount, label: t(".amount"), required: true, disable_currency: true %>
- <%= form.submit t(".submit") %> + <%= form.submit "Update account #{entry.account.asset? ? "value" : "balance"}" %> <% end %> diff --git a/app/views/valuations/new.html.erb b/app/views/valuations/new.html.erb index 82102f16..9d82218d 100644 --- a/app/views/valuations/new.html.erb +++ b/app/views/valuations/new.html.erb @@ -1,6 +1,12 @@ +<% term = @entry.account.asset? ? "value" : "balance" %> + <%= render DialogComponent.new do |dialog| %> - <% dialog.with_header(title: t(".title")) %> + <% dialog.with_header(title: "Account #{term} update") %> <% dialog.with_body do %> <%= render "form", entry: @entry, error_message: @error_message %> + +

+ This action "resets" the account's <%= term %> to the new value, on the date. Subsequent entries after this date will reference the new value. +

<% end %> <% end %> diff --git a/test/controllers/valuations_controller_test.rb b/test/controllers/valuations_controller_test.rb index 52c62ad4..2fee6fd9 100644 --- a/test/controllers/valuations_controller_test.rb +++ b/test/controllers/valuations_controller_test.rb @@ -38,7 +38,7 @@ class ValuationsControllerTest < ActionDispatch::IntegrationTest entry: { amount: 20000, currency: "USD", - date: Date.current + date: @entry.date } } end diff --git a/test/interfaces/entryable_resource_interface_test.rb b/test/interfaces/entryable_resource_interface_test.rb index 710912ac..5e368f48 100644 --- a/test/interfaces/entryable_resource_interface_test.rb +++ b/test/interfaces/entryable_resource_interface_test.rb @@ -4,7 +4,7 @@ module EntryableResourceInterfaceTest extend ActiveSupport::Testing::Declarative test "shows new form" do - get new_polymorphic_url(@entry.entryable) + get new_polymorphic_url(@entry.entryable, account_id: @entry.account_id) assert_response :success end diff --git a/test/models/account/overview_form_test.rb b/test/models/account/overview_form_test.rb index 351cf682..2b40c6b8 100644 --- a/test/models/account/overview_form_test.rb +++ b/test/models/account/overview_form_test.rb @@ -30,6 +30,7 @@ class Account::OverviewFormTest < ActiveSupport::TestCase ) @account.expects(:update!).with(name: "New Property Name").once + @account.expects(:lock_attr!).with(:name).once @account.expects(:sync_later).never # Name change should not trigger sync result = form.save @@ -84,6 +85,7 @@ class Account::OverviewFormTest < ActiveSupport::TestCase # Simulate a validation error on opening balance update @account.expects(:update!).with(name: "New Name").once + @account.expects(:lock_attr!).with(:name).once @account.expects(:set_or_update_opening_balance!).raises(Account::Reconcileable::InvalidBalanceError.new("Cash balance cannot exceed balance")) @account.expects(:sync_later).never # Should NOT sync if any update fails diff --git a/test/models/account/reconcileable_test.rb b/test/models/account/reconcileable_test.rb index 39fa7ade..0b36f891 100644 --- a/test/models/account/reconcileable_test.rb +++ b/test/models/account/reconcileable_test.rb @@ -6,16 +6,9 @@ class Account::ReconcileableTest < ActiveSupport::TestCase @family = families(:dylan_family) end - # Currency updates earn their own method because updating an account currency incurs - # side effects like recalculating balances, etc. - test "can update the account currency" do - @account.update_currency!("EUR") - - assert_equal "EUR", @account.currency - assert_equal "EUR", @account.entries.valuations.first.currency - end - - # If a user has an opening balance (valuation) for their manual account and has 1+ transactions, the intent of + # Scope: Depository Only + # + # If a user has an opening balance (valuation) for their manual *Depository* account and has 1+ transactions, the intent of # "updating current balance" typically means that their start balance is incorrect. We follow that user intent # by default and find the delta required, and update the opening balance so that the timeline reflects this current balance # diff --git a/test/models/account_test.rb b/test/models/account_test.rb index c8eb9749..7716696d 100644 --- a/test/models/account_test.rb +++ b/test/models/account_test.rb @@ -31,4 +31,13 @@ class AccountTest < ActiveSupport::TestCase assert_equal "Investments", account.short_subtype_label assert_equal "Investments", account.long_subtype_label end + + # Currency updates earn their own method because updating an account currency incurs + # side effects like recalculating balances, etc. + test "can update the account currency" do + @account.update_currency!("EUR") + + assert_equal "EUR", @account.currency + assert_equal "EUR", @account.entries.valuations.first.currency + end end diff --git a/test/system/accounts_test.rb b/test/system/accounts_test.rb index e910a3ac..df5f4fcf 100644 --- a/test/system/accounts_test.rb +++ b/test/system/accounts_test.rb @@ -23,20 +23,22 @@ class AccountsTest < ApplicationSystemTestCase end test "can create property account" do - # Step 1: Select property type and enter basic details + # Step 1: Enter basic property details click_link "Property" account_name = "[system test] Property Account" - fill_in "Name*", with: account_name - select "Single Family Home", from: "Property type*" - fill_in "Year Built (optional)", with: 2005 - fill_in "Area (optional)", with: 2250 + fill_in "Name", with: account_name + fill_in "account[current_estimated_value]", with: 500000 + fill_in "account[purchase_price]", with: 450000 + fill_in "account[purchase_date]", with: "01/15/2020" click_button "Next" - # Step 2: Enter balance information - assert_text "Value" - fill_in "account[balance]", with: 500000 + # Step 2: Enter property details + assert_text "Property type" + select "Single Family Home", from: "Property type" + fill_in "Year built", with: 2005 + fill_in "Area", with: 2250 click_button "Next" # Step 3: Enter address information -- 2.53.0 From 13372dea54ab116b1aaf0d4c46d2069753c4a2b3 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Thu, 10 Jul 2025 11:04:08 -0400 Subject: [PATCH 12/12] Reset schema --- db/schema.rb | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index a665850c..df5d7b25 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -29,7 +29,7 @@ ActiveRecord::Schema[7.2].define(version: 2025_07_07_130134) do t.uuid "accountable_id" t.decimal "balance", precision: 19, scale: 4 t.string "currency" - t.virtual "classification", type: :string, as: "\nCASE\n WHEN ((accountable_type)::text = ANY (ARRAY[('Loan'::character varying)::text, ('CreditCard'::character varying)::text, ('OtherLiability'::character varying)::text])) THEN 'liability'::text\n ELSE 'asset'::text\nEND", stored: true + t.virtual "classification", type: :string, as: "\nCASE\n WHEN ((accountable_type)::text = ANY ((ARRAY['Loan'::character varying, 'CreditCard'::character varying, 'OtherLiability'::character varying])::text[])) THEN 'liability'::text\n ELSE 'asset'::text\nEND", stored: true t.uuid "import_id" t.uuid "plaid_account_id" t.decimal "cash_balance", precision: 19, scale: 4, default: "0.0" @@ -215,12 +215,7 @@ ActiveRecord::Schema[7.2].define(version: 2025_07_07_130134) do t.boolean "excluded", default: false t.string "plaid_id" t.jsonb "locked_attributes", default: {} - t.index ["account_id", "date"], name: "index_entries_on_account_id_and_date" t.index ["account_id"], name: "index_entries_on_account_id" - t.index ["amount"], name: "index_entries_on_amount" - t.index ["date"], name: "index_entries_on_date" - t.index ["entryable_id", "entryable_type"], name: "index_entries_on_entryable" - t.index ["excluded"], name: "index_entries_on_excluded" t.index ["import_id"], name: "index_entries_on_import_id" end @@ -231,7 +226,6 @@ ActiveRecord::Schema[7.2].define(version: 2025_07_07_130134) do t.date "date", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.index ["date", "from_currency", "to_currency"], name: "index_exchange_rates_on_date_and_currencies" t.index ["from_currency", "to_currency", "date"], name: "index_exchange_rates_on_base_converted_date_unique", unique: true t.index ["from_currency"], name: "index_exchange_rates_on_from_currency" t.index ["to_currency"], name: "index_exchange_rates_on_to_currency" @@ -689,7 +683,6 @@ ActiveRecord::Schema[7.2].define(version: 2025_07_07_130134) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.index ["tag_id"], name: "index_taggings_on_tag_id" - t.index ["taggable_id", "taggable_type"], name: "index_taggings_on_taggable_id_and_type" t.index ["taggable_type", "taggable_id"], name: "index_taggings_on_taggable" end -- 2.53.0