mirror of
https://github.com/we-promise/sure.git
synced 2026-09-06 15:14:19 +00:00
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:
co-authored by
Claude Opus 5
parent
438a1fba4c
commit
00894c169b
@@ -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?
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user