From 00894c169b3adb620a145d8d4700fd36bd24d0eb Mon Sep 17 00:00:00 2001
From: buzzromain <18685603+buzzromain@users.noreply.github.com>
Date: Mon, 24 Aug 2026 21:48:41 +0000
Subject: [PATCH] fix(goals): treat moving a whole-account link as the fresh
 claim it is
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Addresses review feedback on #3160.

The exclusivity check was bounded to `new_record? ||
will_save_change_to_allocated_amount?`. A persisted whole-account row whose
`account_id` or `goal_id` changes is neither, so moving one landed it on an
account nobody had checked — the same double-counting hole a restore opened,
through a different door.

The bound is widened rather than dropped. It exists because `Goal has_many
:goal_accounts, autosave: true` revalidates every loaded child on `goal.save`,
so an unguarded check makes a goal that merely holds a legacy overlap
impossible to rename. That reason still holds for every row along for the ride;
it does not hold for a row being moved. A test pins both faces.

bin/rails test: 6939 runs, 27916 assertions, 0 failures. RuboCop and Brakeman
clean. Confirmed load-bearing by narrowing the bound back and watching the move
test fail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
---
 app/models/goal_account.rb       | 12 ++++++++-
 test/models/goal_account_test.rb | 42 ++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/app/models/goal_account.rb b/app/models/goal_account.rb
index 0b87b693b..794fc36bc 100644
--- a/app/models/goal_account.rb
+++ b/app/models/goal_account.rb
@@ -39,7 +39,17 @@ class GoalAccount < ApplicationRecord
       # validation stay readable and editable: autosave revalidates every
       # loaded goal_account on goal.save, and blocking there would make a
       # goal that merely holds a legacy overlap impossible to rename.
-      return unless new_record? || will_save_change_to_allocated_amount?
+      #
+      # "Written now" has to include moving the link, not just re-pricing it.
+      # A whole-account row whose account_id or goal_id changes is neither new
+      # nor allocation-dirty, so on those two predicates alone it would land on
+      # an account nobody checked — the same hole as a restore, through a
+      # different door. Widening the bound rather than dropping it: the reason
+      # above still holds for every row that is merely along for the ride.
+      return unless new_record? ||
+                    will_save_change_to_allocated_amount? ||
+                    will_save_change_to_account_id? ||
+                    will_save_change_to_goal_id?
 
       other = goal.whole_account_conflicts_on(account_id).first
       return if other.nil?
diff --git a/test/models/goal_account_test.rb b/test/models/goal_account_test.rb
index bb8637f94..850f0aa8d 100644
--- a/test/models/goal_account_test.rb
+++ b/test/models/goal_account_test.rb
@@ -129,4 +129,46 @@ class GoalAccountTest < ActiveSupport::TestCase
 
     assert_not ga.valid?
   end
+
+  # Moving a whole-account link is a fresh claim on wherever it lands. It is
+  # neither new nor allocation-dirty, so the two original predicates let it
+  # through onto an account nobody had checked.
+  test "moving a whole-account link onto a contested account is refused" do
+    contested = Account.create!(
+      family: families(:dylan_family), accountable: Depository.new,
+      name: "Contested", currency: "USD", balance: 4_000
+    )
+    goals(:vacation_italy).goal_accounts.create!(account: contested)
+    mine = @goal.goal_accounts.create!(account: @account, allocated_amount: nil)
+
+    mine.account = contested
+
+    assert_not mine.valid?
+    assert_includes mine.errors.full_messages.to_sentence, goals(:vacation_italy).name
+  end
+
+  test "moving a whole-account link onto a free account is allowed" do
+    free = Account.create!(
+      family: families(:dylan_family), accountable: Depository.new,
+      name: "Free", currency: "USD", balance: 4_000
+    )
+    mine = @goal.goal_accounts.create!(account: @account, allocated_amount: nil)
+
+    mine.account = free
+
+    assert mine.valid?, mine.errors.full_messages.to_sentence
+  end
+
+  # The bound still has to let a row that is merely along for the ride pass:
+  # autosave revalidates every loaded child on goal.save.
+  test "a legacy overlap still does not block an unrelated edit after the widening" do
+    other = goals(:vacation_italy)
+    other.goal_accounts.create!(account: @account)
+    legacy = @goal.goal_accounts.create!(account: @account, allocated_amount: 1)
+    legacy.update_column(:allocated_amount, nil)
+
+    @goal.reload.name = "Renamed again"
+
+    assert @goal.save, @goal.errors.full_messages.to_sentence
+  end
 end