From f1ddbcd1b5b8cccb45d0270117db6f6b2bb20884 Mon Sep 17 00:00:00 2001 From: Guillem Arias Fauste Date: Tue, 11 Aug 2026 06:17:07 +0200 Subject: [PATCH] fix(goals): recover goals left invalid by account deletion (#2964) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting an account destroys its `goal_accounts` rows (Account has_many :goal_accounts, dependent: :destroy). Any goal funded only by that account survives with zero links and permanently fails `must_have_at_least_one_linked_account`. Two bugs made that state a dead end. `#update` saved the attributes before attaching the submitted accounts, so validation ran against the goal's old (empty) link set and raised before the new links were applied. Editing was the only route back to a valid goal, and it always returned 422. Assign, sync the links, then persist once — one save over the fully assembled goal validates what the user actually submitted. `perform_transition!` discarded the return value of AASM's bang event. AASM returns false rather than raising when the save that persists the new state fails validation, so an invalid goal flashed "Goal archived." while the state never moved. Check the result and surface the validation error instead. Neither fix changes behaviour for a valid goal: the bang events return true on success, and the update path persists the same attributes and links as before. This does not change what happens to a goal when its account is deleted — whether the goal should follow the account, block the deletion, or be surfaced as needing attention is a product decision left open. It only makes the resulting state recoverable and stops the UI reporting success when nothing happened. --- app/controllers/goals_controller.rb | 44 +++++++++++++++------ test/controllers/goals_controller_test.rb | 47 +++++++++++++++++++++++ 2 files changed, 80 insertions(+), 11 deletions(-) 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)