perf + tests(goals): share account-ids across velocity windows + cover gaps

- Family#savings_inflow_windows wraps the current/prior 30d sums in a
  single helper that memoizes the linked-account-id lookup. The KPI tile
  on the goals index used to run the join+pluck twice per request.
- Replace two instance_variable_set pokes and one any_instance.stubs in
  the goal/controller tests. Refetching the goal exercises the real
  request lifecycle and stops the tests from leaning on implementation
  details. The 'All caught up' assertion now relies on a real reached
  state (target 1 vs the depository fixture's 5000 balance) rather than
  stubbing :status.
- Add tests covering: hex format validation on Goal#color, AASM cache
  reset (display_status reads the new state on the same instance after
  pause!), negative pledge amount rejection, expire! no-op on already-
  expired pledge, cancel! NotOpenError on non-open pledge, sweep job
  idempotency on a second pass, and strong-params rejection of state /
  family_id on goal create.

app/controllers/goals_controller.rb

@@ -195,8 +195,9 @@ class GoalsController < ApplicationController
       currency = family.primary_currency_code
       today = Date.current
 
-      velocity_30d = family.savings_inflow_velocity(range: (today - 30)..today)
-      velocity_prior_30d = family.savings_inflow_velocity(range: (today - 60)..(today - 31))
+      windows = family.savings_inflow_windows(window_days: 30, now: today)
+      velocity_30d = windows[:current]
+      velocity_prior_30d = windows[:prior]
       delta_amount = velocity_30d - velocity_prior_30d
       delta_percent = velocity_prior_30d.zero? ? nil : ((delta_amount / velocity_prior_30d.abs) * 100).round(1)
 

app/models/family.rb

@@ -58,22 +58,13 @@ class Family < ApplicationRecord
   # Entry amount convention in Sure: inflow is negative, so flip the sign.
   # Result is allowed to go negative (net outflow last 30d) so the headline
   # reflects reality; the controller decides how to render.
-  def savings_inflow_velocity(range: 30.days.ago.to_date..Date.current)
-    # Defensive scope: goal_id is already family-bound (this family's
-    # goals), but pinning family_id keeps cross-family bleed-through
-    # impossible if a goal_account ever ends up pointing at a foreign
-    # account through a future bug.
-    account_ids = accounts
-      .joins(:goal_accounts)
-      .where(goal_accounts: { goal_id: goals.select(:id) })
-      .where(currency: primary_currency_code)
-      .distinct
-      .pluck(:id)
-    return 0 if account_ids.empty?
+  def savings_inflow_velocity(range: 30.days.ago.to_date..Date.current, account_ids: nil)
+    ids = account_ids || goal_linked_account_ids
+    return 0 if ids.empty?
 
     net = Entry
       .joins("INNER JOIN transactions ON transactions.id = entries.entryable_id AND entries.entryable_type = 'Transaction'")
-      .where(account_id: account_ids, date: range)
+      .where(account_id: ids, date: range)
       .where(excluded: false)
       .merge(Transaction.excluding_pending)
       .sum(:amount)
@@ -81,6 +72,39 @@ class Family < ApplicationRecord
     -net.to_d
   end
 
+  # Two velocity windows in a single pair of sums that share one
+  # account-id lookup. The kpi tile on the index reads both the current
+  # 30d window and the prior 30d window; without this helper the
+  # `accounts.joins(:goal_accounts)…pluck(:id)` query runs twice per
+  # request even though the answer is identical.
+  def savings_inflow_windows(window_days: 30, now: Date.current)
+    ids = goal_linked_account_ids
+    {
+      current: savings_inflow_velocity(range: (now - window_days)..now, account_ids: ids),
+      prior:   savings_inflow_velocity(range: (now - 2 * window_days)..(now - window_days - 1), account_ids: ids)
+    }
+  end
+
+  private
+
+    # Depository accounts linked to this family's goals, restricted to the
+    # primary currency until FX is added. Memoized for the lifetime of the
+    # Family instance so a single request that reads velocity twice (the
+    # KPI tile uses current vs prior 30d) doesn't re-run the join+pluck.
+    # `accounts` is already scoped by the has_many association, and the
+    # join restricts to this family's goals — so cross-family bleed
+    # remains impossible.
+    def goal_linked_account_ids
+      @goal_linked_account_ids ||= accounts
+        .joins(:goal_accounts)
+        .where(goal_accounts: { goal_id: goals.select(:id) })
+        .where(currency: primary_currency_code)
+        .distinct
+        .pluck(:id)
+    end
+
+  public
+
   has_many :llm_usages, dependent: :destroy
   has_many :recurring_transactions, dependent: :destroy
 

test/controllers/goals_controller_test.rb

@@ -165,8 +165,10 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest
   test "index KPI swaps to 'All caught up' when every tracked goal is reached" do
     family = users(:family_admin).family
     family.goals.destroy_all
+    # Real reached state: target $1 against the depository fixture's
+    # $5000 balance. Stubbing :status hides whether the controller
+    # actually reads the right method on each goal.
     build_goal(family, "Wedding", target_amount: 1, target_date: 1.year.from_now)
-    Goal.any_instance.stubs(:status).returns(:reached)
 
     get goals_url
     assert_response :success
@@ -184,6 +186,31 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest
 
   public
 
+  test "create ignores forbidden params (family_id, state)" do
+    family = users(:family_admin).family
+    other_family = Family.create!(name: "Other", currency: "USD", locale: "en", country: "US", timezone: "UTC")
+
+    assert_difference -> { family.goals.count }, 1 do
+      post goals_url, params: {
+        goal: {
+          name: "Hijack target",
+          target_amount: 100,
+          currency: "USD",
+          state: "archived",
+          family_id: other_family.id,
+          account_ids: [ @depository.id ]
+        }
+      }
+    end
+
+    goal = family.goals.order(:created_at).last
+    # Strong params must strip both `state` (AASM-managed) and `family_id`
+    # (cross-family pivot) — otherwise a crafted POST would create rows
+    # outside the current family or skip the active-state assumption.
+    assert_equal "active", goal.state
+    assert_equal family.id, goal.family_id
+  end
+
   test "another family's goal returns 404" do
     other_family = Family.create!(name: "Other", currency: "USD", locale: "en", country: "US", timezone: "UTC")
     other_account = Account.create!(family: other_family, accountable: Depository.new, name: "Foreign", currency: "USD", balance: 100)

test/jobs/sweep_expired_goal_pledges_job_test.rb

@@ -47,4 +47,18 @@ class SweepExpiredGoalPledgesJobTest < ActiveJob::TestCase
 
     assert_nothing_raised { SweepExpiredGoalPledgesJob.perform_now }
   end
+
+  test "second pass is a no-op (idempotent)" do
+    pledge = goal_pledges(:open_transfer)
+    pledge.update_columns(expires_at: 1.day.ago)
+
+    SweepExpiredGoalPledgesJob.perform_now
+    first_updated_at = pledge.reload.updated_at
+
+    travel 1.second do
+      SweepExpiredGoalPledgesJob.perform_now
+      assert_equal first_updated_at.to_i, pledge.reload.updated_at.to_i,
+        "second sweep should not touch a pledge that is already expired"
+    end
+  end
 end

test/models/goal_pledge_test.rb

@@ -138,6 +138,27 @@ class GoalPledgeTest < ActiveSupport::TestCase
     assert_equal 0, pledge.days_left
   end
 
+  test "amount cannot be negative" do
+    @pledge.amount = -5
+    assert_not @pledge.valid?
+    assert_includes @pledge.errors[:amount], "must be greater than 0"
+  end
+
+  test "expire! is a no-op on an already-expired pledge" do
+    @pledge.expire!
+    expired_at = @pledge.updated_at
+    travel 1.second do
+      @pledge.expire!
+      assert_equal expired_at.to_i, @pledge.updated_at.to_i, "second expire! should not touch the row"
+    end
+    assert @pledge.status_expired?
+  end
+
+  test "cancel! raises on non-open pledge" do
+    pledge = goal_pledges(:matched_transfer)
+    assert_raises(GoalPledge::NotOpenError) { pledge.cancel! }
+  end
+
   private
     def build_entry(account:, amount:, date:)
       OpenStruct.new(account_id: account.id, amount: BigDecimal(amount.to_s), date: date.to_date)

test/models/goal_test.rb

@@ -25,6 +25,24 @@ class GoalTest < ActiveSupport::TestCase
     assert_not @goal.valid?
   end
 
+  test "color must match hex format" do
+    @goal.color = "red; cursor: pointer"
+    assert_not @goal.valid?
+    assert_includes @goal.errors[:color], "is invalid"
+  end
+
+  test "color accepts standard 6-digit hex" do
+    @goal.color = "#abcdef"
+    assert @goal.valid?, @goal.errors.full_messages.to_sentence
+  end
+
+  test "display_status follows AASM state after pause! on the same instance" do
+    @goal.update!(color: "#4da568") if @goal.color.blank?
+    initial = @goal.display_status
+    @goal.pause!
+    assert_equal :paused, @goal.display_status, "stale memo would have returned #{initial.inspect}"
+  end
+
   test "must have at least one linked account on create" do
     new_goal = @family.goals.new(name: "Test", target_amount: 100, currency: "USD")
     assert_not new_goal.valid?
@@ -87,11 +105,13 @@ class GoalTest < ActiveSupport::TestCase
 
   test "progress_percent is 0 for empty active goal" do
     fresh = goals(:car_paydown)
-    fresh.target_amount = 10_000
+    fresh.update!(target_amount: 10_000)
     fresh.linked_accounts.update_all(balance: 0)
-    fresh.instance_variable_set(:@current_balance, nil)
-    fresh.linked_accounts.reload
-    assert_equal 0, fresh.progress_percent
+    # Refetch instead of poking @current_balance directly so the test
+    # exercises the real memo lifecycle (a request reads progress_percent
+    # on a freshly-loaded record after the underlying balances changed).
+    reloaded = Goal.find(fresh.id)
+    assert_equal 0, reloaded.progress_percent
   end
 
   test "remaining_amount is non-negative" do
@@ -221,11 +241,11 @@ class GoalTest < ActiveSupport::TestCase
   test "pledge_action_label_key flips on manual-only goals" do
     assert_equal "goals.show.pledge_just_transferred", @goal.pledge_action_label_key
     @goal.goal_accounts.where(account_id: @connected.id).destroy_all
-    @goal.reload
-    @goal.instance_variable_set(:@current_balance, nil)
     # After removing the only connected account, the goal is manual-only;
     # the copy must flip to "pledge_just_saved" so users aren't told to
-    # wait for a sync that won't run.
-    assert_equal "goals.show.pledge_just_saved", @goal.pledge_action_label_key
+    # wait for a sync that won't run. Refetch to exercise the real
+    # request lifecycle rather than poking a memo on the same instance.
+    reloaded = Goal.find(@goal.id)
+    assert_equal "goals.show.pledge_just_saved", reloaded.pledge_action_label_key
   end
 end