Make sure we only detect activity spikes when we should

The detection logic wasn't really working before

app/models/card/engageable.rb

@@ -44,7 +44,7 @@ module Card::Engageable
     transaction do
       reopen
       engagement&.destroy
-      touch(:last_active_at)
+      touch_last_active_at
     end
   end
 end

app/models/card/eventable.rb

@@ -12,10 +12,15 @@ module Card::Eventable
   def event_was_created(event)
     transaction do
       create_system_comment_for(event)
-      touch(:last_active_at)
+      touch_last_active_at
     end
   end
 
+  def touch_last_active_at
+    # Not using touch so that we can detect attribute change on callbacks
+    update!(last_active_at: Time.current)
+  end
+
   private
     def should_track_event?
       published?

app/models/card/stallable.rb

@@ -9,7 +9,8 @@ module Card::Stallable
     scope :with_activity_spikes, -> { joins(:activity_spike) }
     scope :stalled, -> { open.with_activity_spikes.where("card_activity_spikes.updated_at": ..STALLED_AFTER_LAST_SPIKE_PERIOD.ago) }
 
-    after_update_commit :detect_activity_spikes_later, if: :saved_change_to_last_active_at?
+    before_update :remember_to_detect_activity_spikes
+    after_update_commit :detect_activity_spikes_later, if: :should_detect_activity_spikes?
   end
 
   def stalled?
@@ -25,6 +26,14 @@ module Card::Stallable
   end
 
   private
+    def remember_to_detect_activity_spikes
+      @should_detect_activity_spikes = published? && last_active_at_changed?
+    end
+
+    def should_detect_activity_spikes?
+      @should_detect_activity_spikes
+    end
+
     def detect_activity_spikes_later
       Card::ActivitySpike::DetectionJob.perform_later(self)
     end

app/models/comment/eventable.rb

@@ -8,7 +8,7 @@ module Comment::Eventable
   end
 
   def event_was_created(event)
-    card.touch(:last_active_at)
+    card.touch_last_active_at
   end
 
   private

test/fixtures/cards.yml

@@ -6,7 +6,7 @@ logo:
   due_on: <%= 3.days.from_now %>
   created_at: <%= 1.week.ago %>
   status: published
-  last_active_at: <%= Time.current %>
+  last_active_at: <%= 1.week.ago %>
 
 layout:
   collection: writebook
@@ -15,16 +15,15 @@ layout:
   title: Layout is broken
   created_at: <%= 1.week.ago %>
   status: published
-  last_active_at: <%= Time.current %>
+  last_active_at: <%= 1.week.ago %>
 
 text:
   collection: writebook
   creator: kevin
-  stage: qa_triage
   title: The text is too small
   created_at: <%= 1.week.ago %>
   status: published
-  last_active_at: <%= Time.current %>
+  last_active_at: <%= 1.week.ago %>
   stage: qa_in_progress
 
 shipping:
@@ -34,4 +33,4 @@ shipping:
   title: We need to ship the app
   created_at: <%= 1.week.ago %>
   status: published
-  last_active_at: <%= Time.current %>
+  last_active_at: <%= 1.week.ago %>

test/models/card/stallable_test.rb

@@ -35,4 +35,16 @@ class Card::StallableTest < ActiveSupport::TestCase
     assert cards(:logo).reload.stalled?
     assert_includes Card.stalled, cards(:logo)
   end
+
+  test "don't detect activity spikes when updating attributes other than last_active_at" do
+    assert_no_enqueued_jobs only: Card::ActivitySpike::DetectionJob do
+      cards(:logo).update! created_at: 1.day.ago
+    end
+  end
+
+  test "don't detect activity spikes when creating new cards" do
+    assert_no_enqueued_jobs only: Card::ActivitySpike::DetectionJob do
+      collections(:writebook).cards.create! title: "A new card", creator: users(:kevin)
+    end
+  end
 end