From d80cb9f8128451b6cbcf0a32d33871c5ffba8d87 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Thu, 10 Jul 2025 10:31:40 -0400 Subject: [PATCH] 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