fix(goals): treat moving a whole-account link as the fresh claim it is

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
This commit is contained in:
buzzromain
2026-08-24 21:48:41 +00:00
co-authored by Claude Opus 5
parent 438a1fba4c
commit 00894c169b
2 changed files with 53 additions and 1 deletions
+11 -1
View File
@@ -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?
+42
View File
@@ -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