diff --git a/app/controllers/valuations_controller.rb b/app/controllers/valuations_controller.rb index 85f3413a..5234c0be 100644 --- a/app/controllers/valuations_controller.rb +++ b/app/controllers/valuations_controller.rb @@ -5,6 +5,12 @@ class ValuationsController < ApplicationController @account = Current.family.accounts.find(params.dig(:entry, :account_id)) @entry = @account.entries.build(entry_params.merge(currency: @account.currency)) + @reconciliation_dry_run = @entry.account.create_reconciliation( + balance: entry_params[:amount], + date: entry_params[:date], + dry_run: true + ) + render :confirm_create end @@ -13,19 +19,28 @@ class ValuationsController < ApplicationController @account = @entry.account @entry.assign_attributes(entry_params.merge(currency: @account.currency)) + @reconciliation_dry_run = @entry.account.update_reconciliation( + @entry, + balance: entry_params[:amount], + date: entry_params[:date], + dry_run: true + ) + render :confirm_update end def create account = Current.family.accounts.find(params.dig(:entry, :account_id)) - result = perform_balance_update(account, entry_params.merge(currency: account.currency)) + + result = account.create_reconciliation( + balance: entry_params[:amount], + date: entry_params[:date], + ) 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) } + format.html { redirect_back_or_to account_path(account), notice: "Account updated" } + format.turbo_stream { stream_redirect_back_or_to(account_path(account), notice: "Account updated") } end else @error_message = result.error_message @@ -34,13 +49,22 @@ class ValuationsController < ApplicationController end def update - result = perform_balance_update(@entry.account, entry_params.merge(currency: @entry.currency, existing_valuation_id: @entry.id)) + # Notes updating is independent of reconciliation, just a simple CRUD operation + @entry.update!(notes: entry_params[:notes]) if entry_params[:notes].present? - if result.success? + if entry_params[:date].present? && entry_params[:amount].present? + result = @entry.account.update_reconciliation( + @entry, + balance: entry_params[:amount], + date: entry_params[:date], + ) + end + + if result.nil? || result.success? @entry.reload 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.html { redirect_back_or_to account_path(@entry.account), notice: "Entry updated" } format.turbo_stream do render turbo_stream: [ turbo_stream.replace( @@ -60,17 +84,6 @@ class ValuationsController < ApplicationController private def entry_params - params.require(:entry) - .permit(:date, :amount, :currency, :notes) - end - - def perform_balance_update(account, params) - account.update_balance( - balance: params[:amount], - date: params[:date], - currency: params[:currency], - notes: params[:notes], - existing_valuation_id: params[:existing_valuation_id] - ) + params.require(:entry).permit(:date, :amount, :notes) end end diff --git a/app/models/account.rb b/app/models/account.rb index 837d6e57..684736ce 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -1,5 +1,5 @@ class Account < ApplicationRecord - include AASM, Syncable, Monetizable, Chartable, Linkable, Enrichable, Anchorable + include AASM, Syncable, Monetizable, Chartable, Linkable, Enrichable, Anchorable, Reconcileable validates :name, :balance, :currency, presence: true diff --git a/app/models/account/reconcileable.rb b/app/models/account/reconcileable.rb new file mode 100644 index 00000000..bad855a9 --- /dev/null +++ b/app/models/account/reconcileable.rb @@ -0,0 +1,16 @@ +module Account::Reconcileable + extend ActiveSupport::Concern + + def create_reconciliation(balance:, date:, dry_run: false) + reconciliation_manager.reconcile_balance(balance: balance, date: date, dry_run: dry_run) + end + + def update_reconciliation(existing_valuation_entry, balance:, date:, dry_run: false) + reconciliation_manager.reconcile_balance(balance: balance, date: date, existing_valuation_entry: existing_valuation_entry, dry_run: dry_run) + end + + private + def reconciliation_manager + @reconciliation_manager ||= Account::ReconciliationManager.new(self) + end +end diff --git a/app/models/account/reconciliation_manager.rb b/app/models/account/reconciliation_manager.rb new file mode 100644 index 00000000..e1300143 --- /dev/null +++ b/app/models/account/reconciliation_manager.rb @@ -0,0 +1,90 @@ +class Account::ReconciliationManager + attr_reader :account + + def initialize(account) + @account = account + end + + # Reconciles balance by creating a Valuation entry. If existing valuation is provided, it will be updated instead of creating a new one. + def reconcile_balance(balance:, date: Date.current, dry_run: false, existing_valuation_entry: nil) + old_balance_components = old_balance_components(reconciliation_date: date, existing_valuation_entry: existing_valuation_entry) + prepared_valuation = prepare_reconciliation(balance, date, existing_valuation_entry) + + unless dry_run + prepared_valuation.save! + account.sync_later + end + + ReconciliationResult.new( + success?: true, + old_cash_balance: old_balance_components[:cash_balance], + old_balance: old_balance_components[:balance], + new_cash_balance: derived_cash_balance(date: date, total_balance: prepared_valuation.amount), + new_balance: prepared_valuation.amount, + error_message: nil + ) + rescue => e + ReconciliationResult.new( + success?: false, + error_message: e.message + ) + end + + private + # Returns before -> after OR error message + ReconciliationResult = Struct.new( + :success?, + :old_cash_balance, + :old_balance, + :new_cash_balance, + :new_balance, + :error_message, + keyword_init: true + ) + + def prepare_reconciliation(balance, date, existing_valuation) + valuation_record = existing_valuation || + account.entries.valuations.find_by(date: date) || # In case of conflict, where existing valuation is not passed as arg, but one exists + account.entries.build( + name: Valuation.build_reconciliation_name(account.accountable_type), + entryable: Valuation.new(kind: "reconciliation") + ) + + valuation_record.assign_attributes( + date: date, + amount: balance, + currency: account.currency + ) + + valuation_record + end + + def derived_cash_balance(date:, total_balance:) + balance_components_for_reconciliation_date = get_balance_components_for_date(date) + + return nil unless balance_components_for_reconciliation_date[:balance] && balance_components_for_reconciliation_date[:cash_balance] + + # We calculate the existing non-cash balance, which for investments would represents "holdings" for the date of reconciliation + # Since the user is setting "total balance", we have to subtract the existing non-cash balance from the total balance to get the new cash balance + existing_non_cash_balance = balance_components_for_reconciliation_date[:balance] - balance_components_for_reconciliation_date[:cash_balance] + + total_balance - existing_non_cash_balance + end + + def old_balance_components(reconciliation_date:, existing_valuation_entry: nil) + if existing_valuation_entry + get_balance_components_for_date(existing_valuation_entry.date) + else + get_balance_components_for_date(reconciliation_date) + end + end + + def get_balance_components_for_date(date) + balance_record = account.balances.find_by(date: date, currency: account.currency) + + { + cash_balance: balance_record&.cash_balance, + balance: balance_record&.balance + } + end +end diff --git a/app/models/investment.rb b/app/models/investment.rb index 10d879e4..4e4c25c8 100644 --- a/app/models/investment.rb +++ b/app/models/investment.rb @@ -28,19 +28,4 @@ class Investment < ApplicationRecord "line-chart" end end - - def holdings_value_for_date(date) - # Find the most recent holding for each security on or before the given date - # Using a subquery to get the max date for each security - account.holdings - .where(currency: account.currency) - .where("date <= ?", date) - .where("(security_id, date) IN ( - SELECT security_id, MAX(date) as max_date - FROM holdings - WHERE account_id = ? AND date <= ? - GROUP BY security_id - )", account.id, date) - .sum(:amount) - end end diff --git a/app/views/valuations/_confirmation_contents.html.erb b/app/views/valuations/_confirmation_contents.html.erb index c3dc3724..19d2ff5f 100644 --- a/app/views/valuations/_confirmation_contents.html.erb +++ b/app/views/valuations/_confirmation_contents.html.erb @@ -1,14 +1,16 @@ +<%# locals: (account:, entry:, reconciliation_dry_run:, is_update:, action_verb:) %> +
<% if account.investment? %> - <% holdings_value = account.investment.holdings_value_for_date(entry.date) %> - <% brokerage_cash = entry.amount - holdings_value %> + <% holdings_value = reconciliation_dry_run.new_balance - reconciliation_dry_run.new_cash_balance %> + <% brokerage_cash = reconciliation_dry_run.new_cash_balance %>

This will <%= action_verb %> the account value on <%= entry.date.strftime("%B %d, %Y") %> to:

Total account value - <%= entry.amount_money.format %> + <%= Money.new(reconciliation_dry_run.new_balance, account.currency).format %>
Holdings value @@ -20,7 +22,7 @@
<% else %> -

<%= action_verb.capitalize %> +

<%= action_verb.capitalize %> <% if account.depository? %> account balance <% elsif account.credit_card? %> @@ -40,10 +42,10 @@ <% else %> balance <% end %> - on <%= entry.date.strftime("%B %d, %Y") %> to + on <%= entry.date.strftime("%B %d, %Y") %> to <%= entry.amount_money.format %>.

<% end %>

All future transactions and balances will be recalculated based on this <%= is_update ? "change" : "update" %>.

-
\ No newline at end of file + diff --git a/app/views/valuations/confirm_create.html.erb b/app/views/valuations/confirm_create.html.erb index 75646450..3e3a4a90 100644 --- a/app/views/valuations/confirm_create.html.erb +++ b/app/views/valuations/confirm_create.html.erb @@ -1,17 +1,16 @@ <%= render DialogComponent.new do |dialog| %> <% dialog.with_header(title: "Confirm new balance") %> <% dialog.with_body do %> - <%= styled_form_with model: @entry, url: valuations_path, class: "space-y-4", data: { turbo: false } do |form| %> + <%= styled_form_with model: @entry, url: valuations_path, class: "space-y-4" do |form| %> <%= form.hidden_field :account_id %> <%= form.hidden_field :date %> <%= form.hidden_field :amount %> - <%= form.hidden_field :currency %> - <%= form.hidden_field :notes %> - <%= render "confirmation_contents", - account: @account, - entry: @entry, - action_verb: "set", + <%= render "confirmation_contents", + reconciliation_dry_run: @reconciliation_dry_run, + account: @account, + entry: @entry, + action_verb: "set", is_update: false %> <%= form.submit "Confirm" %> diff --git a/app/views/valuations/confirm_update.html.erb b/app/views/valuations/confirm_update.html.erb index 43c67c37..c24e27cf 100644 --- a/app/views/valuations/confirm_update.html.erb +++ b/app/views/valuations/confirm_update.html.erb @@ -1,19 +1,18 @@ <%= render DialogComponent.new do |dialog| %> <% dialog.with_header(title: "Update balance") %> <% dialog.with_body do %> - <%= styled_form_with model: @entry, url: valuation_path(@entry), method: :patch, class: "space-y-4", data: { turbo: false } do |form| %> + <%= styled_form_with model: @entry, url: valuation_path(@entry), method: :patch, class: "space-y-4", data: { turbo_frame: :_top } do |form| %> <%= form.hidden_field :date %> <%= form.hidden_field :amount %> - <%= form.hidden_field :currency %> - <%= form.hidden_field :notes %> - <%= render "confirmation_contents", - account: @account, - entry: @entry, - action_verb: "update", + <%= render "confirmation_contents", + reconciliation_dry_run: @reconciliation_dry_run, + account: @account, + entry: @entry, + action_verb: "update", is_update: true %> <%= form.submit "Update" %> <% end %> <% end %> -<% end %> \ No newline at end of file +<% end %> diff --git a/app/views/valuations/show.html.erb b/app/views/valuations/show.html.erb index b4156e39..3cd44f9f 100644 --- a/app/views/valuations/show.html.erb +++ b/app/views/valuations/show.html.erb @@ -44,10 +44,7 @@ url: valuation_path(entry), method: :patch, class: "space-y-2", - data: { controller: "auto-submit-form" } do |f| %> - <%= f.hidden_field :date, value: entry.date %> - <%= f.hidden_field :amount, value: entry.amount %> - <%= f.hidden_field :currency, value: entry.currency %> + data: { controller: "auto-submit-form", auto_submit_form_trigger_event_value: "blur" } do |f| %> <%= f.text_area :notes, label: t(".note_label"), placeholder: t(".note_placeholder"), diff --git a/test/controllers/valuations_controller_test.rb b/test/controllers/valuations_controller_test.rb index 4746eb32..7827906b 100644 --- a/test/controllers/valuations_controller_test.rb +++ b/test/controllers/valuations_controller_test.rb @@ -8,7 +8,7 @@ class ValuationsControllerTest < ActionDispatch::IntegrationTest @entry = entries(:valuation) end - test "creates entry with basic attributes" do + test "can create reconciliation" do account = accounts(:investment) assert_difference [ "Entry.count", "Valuation.count" ], 1 do @@ -35,8 +35,9 @@ class ValuationsControllerTest < ActionDispatch::IntegrationTest assert_no_difference [ "Entry.count", "Valuation.count" ] do patch valuation_url(@entry), params: { entry: { - amount: 20000, - date: Date.current + amount: 22000, + date: Date.current, + notes: "Test notes" } } end @@ -44,5 +45,9 @@ class ValuationsControllerTest < ActionDispatch::IntegrationTest assert_enqueued_with job: SyncJob assert_redirected_to account_url(@entry.account) + + @entry.reload + assert_equal 22000, @entry.amount + assert_equal "Test notes", @entry.notes end end diff --git a/test/models/account/reconciliation_manager_test.rb b/test/models/account/reconciliation_manager_test.rb new file mode 100644 index 00000000..0e7a39a9 --- /dev/null +++ b/test/models/account/reconciliation_manager_test.rb @@ -0,0 +1,103 @@ +require "test_helper" + +class Account::ReconciliationManagerTest < ActiveSupport::TestCase + setup do + @account = accounts(:investment) + @manager = Account::ReconciliationManager.new(@account) + end + + test "new reconciliation" do + @account.balances.create!( + date: Date.current, + balance: 1000, + cash_balance: 500, + currency: @account.currency + ) + + result = @manager.reconcile_balance(balance: 1200, date: Date.current) + + assert_equal 1200, result.new_balance + assert_equal 700, result.new_cash_balance # Non cash stays the same since user is valuing the entire account balance + assert_equal 1000, result.old_balance + assert_equal 500, result.old_cash_balance + assert_equal true, result.success? + end + + test "updates existing reconciliation without date change" do + @account.balances.create!(date: Date.current, balance: 1000, cash_balance: 500, currency: @account.currency) + + # Existing reconciliation entry + existing_entry = @account.entries.create!(name: "Test", amount: 1000, date: Date.current, entryable: Valuation.new(kind: "reconciliation"), currency: @account.currency) + + result = @manager.reconcile_balance(balance: 1200, date: Date.current, existing_valuation_entry: existing_entry) + + assert_equal 1200, result.new_balance + assert_equal 700, result.new_cash_balance # Non cash stays the same since user is valuing the entire account balance + assert_equal 1000, result.old_balance + assert_equal 500, result.old_cash_balance + assert_equal true, result.success? + end + + test "updates existing reconciliation with date and amount change" do + @account.balances.create!(date: 5.days.ago, balance: 1000, cash_balance: 500, currency: @account.currency) + @account.balances.create!(date: Date.current, balance: 1200, cash_balance: 700, currency: @account.currency) + + # Existing reconciliation entry (5 days ago) + existing_entry = @account.entries.create!(name: "Test", amount: 1000, date: 5.days.ago, entryable: Valuation.new(kind: "reconciliation"), currency: @account.currency) + + # Should update and change date for existing entry; not create a new one + assert_no_difference "Valuation.count" do + # "Update valuation from 5 days ago to today, set balance from 1000 to 1500" + result = @manager.reconcile_balance(balance: 1500, date: Date.current, existing_valuation_entry: existing_entry) + + assert_equal true, result.success? + + # Reconciliation + assert_equal 1500, result.new_balance # Equal to new valuation amount + assert_equal 1000, result.new_cash_balance # Get non-cash balance today (1200 - 700 = 500). Then subtract this from new valuation (1500 - 500 = 1000) + + # Prior valuation + assert_equal 1000, result.old_balance # This is the balance from the old valuation, NOT the date we're reconciling to + assert_equal 500, result.old_cash_balance + end + end + + test "handles date conflicts" do + @account.balances.create!( + date: Date.current, + balance: 1000, + cash_balance: 1000, + currency: @account.currency + ) + + # Existing reconciliation entry + @account.entries.create!( + name: "Test", + amount: 1000, + date: Date.current, + entryable: Valuation.new(kind: "reconciliation"), + currency: @account.currency + ) + + # Doesn't pass existing_valuation_entry, but reconciliation manager should recognize its the same date and update the existing entry + assert_no_difference "Valuation.count" do + result = @manager.reconcile_balance(balance: 1200, date: Date.current) + + assert result.success? + assert_equal 1200, result.new_balance + end + end + + test "dry run does not persist or sync account" do + @account.balances.create!(date: Date.current, balance: 1000, cash_balance: 500, currency: @account.currency) + + assert_no_difference "Valuation.count" do + @manager.reconcile_balance(balance: 1200, date: Date.current, dry_run: true) + end + + assert_difference "Valuation.count", 1 do + @account.expects(:sync_later).once + @manager.reconcile_balance(balance: 1200, date: Date.current) + end + end +end