diff --git a/app/controllers/goals_controller.rb b/app/controllers/goals_controller.rb index ddb3896eb..5de8aa24a 100644 --- a/app/controllers/goals_controller.rb +++ b/app/controllers/goals_controller.rb @@ -93,9 +93,20 @@ class GoalsController < ApplicationController return end + # Assign first, sync the links, then persist once. Saving the attributes + # up front ran `must_have_at_least_one_linked_account` against the goal's + # OLD links, so a goal orphaned by account deletion (Account has_many + # :goal_accounts, dependent: :destroy) could never be edited back to + # health: the save raised before the submitted accounts were attached. + # One save over the fully-assembled goal validates what the user actually + # submitted. Goal.transaction do - @goal.update!(goal_update_params) - sync_linked_accounts!(@goal, accounts, submitted_allocations) if accounts_supplied + @goal.assign_attributes(goal_update_params) + if accounts_supplied + sync_linked_accounts!(@goal, accounts, submitted_allocations) + else + @goal.save! + end end flash[:notice] = t(".success") @@ -289,16 +300,27 @@ class GoalsController < ApplicationController end def perform_transition!(event) - if @goal.aasm.may_fire_event?(event) - @goal.public_send("#{event}!") - respond_to do |format| - format.html { redirect_to goal_path(@goal), notice: t(".success") } - format.turbo_stream do - render turbo_stream: turbo_stream.action(:redirect, goal_path(@goal)) - end - end - else + unless @goal.aasm.may_fire_event?(event) redirect_to goal_path(@goal), alert: t(".invalid_transition") + return + end + + # AASM's bang event returns false — it does NOT raise — when the save + # that persists the new state fails validation. The return value used to + # be discarded, so an invalid goal (e.g. orphaned by account deletion) + # flashed "Goal archived." while the state never moved. Surface the + # validation error instead of claiming success. + unless @goal.public_send("#{event}!") + redirect_to goal_path(@goal), + alert: @goal.errors.full_messages.to_sentence.presence || t(".invalid_transition") + return + end + + respond_to do |format| + format.html { redirect_to goal_path(@goal), notice: t(".success") } + format.turbo_stream do + render turbo_stream: turbo_stream.action(:redirect, goal_path(@goal)) + end end end end diff --git a/test/controllers/goals_controller_test.rb b/test/controllers/goals_controller_test.rb index 2fb08236c..cfc5c6d5b 100644 --- a/test/controllers/goals_controller_test.rb +++ b/test/controllers/goals_controller_test.rb @@ -210,6 +210,36 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest assert fresh.reload.active? end + # A goal whose last funding account is deleted survives with zero links and + # fails `must_have_at_least_one_linked_account` from then on. Editing is the + # only way back, so update must validate the accounts the user SUBMITTED, + # not the stale (empty) set already on the record. + test "an orphaned goal can be repaired by re-linking an account" do + orphan = orphaned_goal + + patch goal_url(orphan), params: { + goal: { name: orphan.name, target_amount: orphan.target_amount, account_ids: [ @depository.id ] } + } + + assert_redirected_to goal_path(orphan) + assert_equal [ @depository.id ], orphan.reload.goal_accounts.pluck(:account_id) + assert orphan.valid? + end + + # AASM's bang event returns false rather than raising when the post-transition + # save fails validation. The controller used to discard that, flashing + # "Goal archived." while the state never moved. + test "a transition that fails validation reports the error, not success" do + orphan = orphaned_goal + + patch archive_goal_url(orphan) + + assert_redirected_to goal_path(orphan) + assert_nil flash[:notice] + assert_match(/at least one account/i, flash[:alert]) + assert_equal "active", orphan.reload.state + end + test "destroy on non-archived is rejected" do assert_no_difference "Goal.count" do delete goal_url(@goal) @@ -257,6 +287,23 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest end private + # A goal in the state account deletion leaves behind: still present, zero + # linked accounts, failing its own validations. + def orphaned_goal + family = @user.family + throwaway = Account.create!( + family: family, accountable: Depository.new, name: "Throwaway", currency: "USD", balance: 100 + ) + goal = family.goals.new(name: "Orphan", target_amount: 500, currency: "USD") + goal.goal_accounts.build(account: throwaway) + goal.save! + + throwaway.destroy! + goal.reload + assert_empty goal.goal_accounts, "fixture setup failed to orphan the goal" + goal + end + def build_goal(family, name, target_amount: 1_000_000, target_date: nil) g = family.goals.new(name: name, target_amount: target_amount, target_date: target_date, currency: "USD") g.goal_accounts.build(account: @depository)