FIX: resets pending automations only if necessary (#26685)

Prior to this fix, any change to an automation would reset `pending_automations`, now we only do it if any value related to recurrence (start_date, interval, frequency, execute_at...) has been changed.

It means that any trigger creating `pending_automations` now needs to manage them in the `on_update` callback.

plugins/automation/app/models/discourse_automation/automation.rb

@@ -162,7 +162,6 @@ module DiscourseAutomation
     end
 
     def reset!
-      pending_automations.delete_all
       pending_pms.delete_all
       scriptable&.on_reset&.call(self)
     end

plugins/automation/lib/discourse_automation/triggers/point_in_time.rb

@@ -3,11 +3,17 @@
 DiscourseAutomation::Triggerable.add(DiscourseAutomation::Triggers::POINT_IN_TIME) do
   field :execute_at, component: :date_time, required: true
 
-  on_update do |automation, metadata|
-    # prevents creating a new pending automation on save when date is expired
-    execute_at = metadata.dig("execute_at", "value")
-    if execute_at && execute_at > Time.zone.now
-      automation.pending_automations.create!(execute_at: execute_at)
+  on_update do |automation, fields, previous_fields|
+    execute_at = fields.dig("execute_at", "value")
+    previous_execute_at = previous_fields&.dig("execute_at", "value")
+
+    if execute_at != previous_execute_at
+      automation.pending_automations.destroy_all
+
+      # prevents creating a new pending automation on save when date is expired
+      if execute_at && execute_at > Time.zone.now
+        automation.pending_automations.create!(execute_at: execute_at)
+      end
     end
   end
 end

plugins/automation/lib/discourse_automation/triggers/recurring.rb

@@ -13,12 +13,27 @@ module DiscourseAutomation
         { id: "year", name: "discourse_automation.triggerables.recurring.frequencies.year" },
       ]
 
-      def self.setup_pending_automation(automation, fields)
-        automation.pending_automations.destroy_all
+      def self.setup_pending_automation(automation, fields, previous_fields)
+        start_date = fields.dig("start_date", "value")
+        interval = fields.dig("recurrence", "value", "interval")
+        frequency = fields.dig("recurrence", "value", "frequency")
 
-        return unless start_date = fields.dig("start_date", "value")
-        return unless interval = fields.dig("recurrence", "value", "interval")
-        return unless frequency = fields.dig("recurrence", "value", "frequency")
+        # this case is not possible in practice but better be safe
+        if !start_date || !interval || !frequency
+          automation.pending_automations.destroy_all
+          return
+        end
+
+        previous_start_date = previous_fields&.dig("start_date", "value")
+        previous_interval = previous_fields&.dig("recurrence", "value", "interval")
+        previous_frequency = previous_fields&.dig("recurrence", "value", "frequency")
+
+        if previous_start_date != start_date || previous_interval != interval ||
+             previous_frequency != frequency
+          automation.pending_automations.destroy_all
+        else
+          return
+        end
 
         start_date = Time.parse(start_date)
         byday = start_date.strftime("%A").upcase[0, 2]
@@ -82,11 +97,19 @@ DiscourseAutomation::Triggerable.add(DiscourseAutomation::Triggers::RECURRING) d
         required: true
   field :start_date, component: :date_time, required: true
 
-  on_update do |automation, fields|
-    DiscourseAutomation::Triggers::Recurring.setup_pending_automation(automation, fields)
+  on_update do |automation, fields, previous_fields|
+    DiscourseAutomation::Triggers::Recurring.setup_pending_automation(
+      automation,
+      fields,
+      previous_fields,
+    )
   end
-  on_call do |automation, fields|
-    DiscourseAutomation::Triggers::Recurring.setup_pending_automation(automation, fields)
+  on_call do |automation, fields, previous_fields|
+    DiscourseAutomation::Triggers::Recurring.setup_pending_automation(
+      automation,
+      fields,
+      previous_fields,
+    )
   end
 
   enable_manual_trigger

plugins/automation/spec/triggers/point_in_time_spec.rb

@@ -40,4 +40,49 @@ describe "PointInTime" do
       end
     end
   end
+
+  context "when updating automation" do
+    fab!(:automation) do
+      Fabricate(:automation, trigger: DiscourseAutomation::Triggers::POINT_IN_TIME, script: "test")
+    end
+
+    before do
+      DiscourseAutomation::Scriptable.add("test") do
+        triggerables [DiscourseAutomation::Triggers::POINT_IN_TIME]
+        field :test, component: :text
+      end
+
+      automation.upsert_field!(
+        "execute_at",
+        "date_time",
+        { value: 2.hours.from_now },
+        target: "trigger",
+      )
+
+      automation.upsert_field!("test", "text", { value: "something" }, target: "script")
+    end
+
+    context "when execute_at changes" do
+      it "resets the pending automations" do
+        expect {
+          automation.upsert_field!(
+            "execute_at",
+            "date_time",
+            { value: 3.hours.from_now },
+            target: "trigger",
+          )
+        }.to change { DiscourseAutomation::PendingAutomation.last.execute_at }
+        expect(DiscourseAutomation::PendingAutomation.count).to eq(1)
+      end
+    end
+
+    context "when a field other than execute_at changes" do
+      it "doesn't reset the pending automations" do
+        expect {
+          automation.upsert_field!("test", "text", { value: "somethingelse" }, target: "script")
+        }.to_not change { DiscourseAutomation::PendingAutomation.last.execute_at }
+        expect(DiscourseAutomation::PendingAutomation.count).to eq(1)
+      end
+    end
+  end
 end

plugins/automation/spec/triggers/recurring_spec.rb

@@ -58,6 +58,70 @@ describe "Recurring" do
     end
   end
 
+  context "when updating automation" do
+    fab!(:automation) do
+      Fabricate(:automation, trigger: DiscourseAutomation::Triggers::RECURRING, script: "test")
+    end
+
+    before do
+      DiscourseAutomation::Scriptable.add("test") do
+        triggerables [DiscourseAutomation::Triggers::RECURRING]
+        field :test, component: :text
+      end
+
+      automation.upsert_field!(
+        "start_date",
+        "date_time",
+        { value: 2.hours.from_now },
+        target: "trigger",
+      )
+      upsert_period_field!(1, "week")
+
+      automation.upsert_field!("test", "text", { value: "something" }, target: "script")
+    end
+
+    context "when start date changes" do
+      it "resets the pending automations" do
+        expect {
+          automation.upsert_field!(
+            "start_date",
+            "date_time",
+            { value: 1.day.from_now },
+            target: "trigger",
+          )
+        }.to change { DiscourseAutomation::PendingAutomation.last.execute_at }
+        expect(DiscourseAutomation::PendingAutomation.count).to eq(1)
+      end
+    end
+
+    context "when interval changes" do
+      it "resets the pending automations" do
+        expect { upsert_period_field!(2, "week") }.to change {
+          DiscourseAutomation::PendingAutomation.last.execute_at
+        }
+        expect(DiscourseAutomation::PendingAutomation.count).to eq(1)
+      end
+    end
+
+    context "when frequency changes" do
+      it "resets the pending automations" do
+        expect { upsert_period_field!(1, "month") }.to change {
+          DiscourseAutomation::PendingAutomation.last.execute_at
+        }
+        expect(DiscourseAutomation::PendingAutomation.count).to eq(1)
+      end
+    end
+
+    context "when a non recurrence related field changes" do
+      it "doesn't reset the pending automations" do
+        expect {
+          automation.upsert_field!("test", "text", { value: "somethingelse" }, target: "script")
+        }.to_not change { DiscourseAutomation::PendingAutomation.last.execute_at }
+        expect(DiscourseAutomation::PendingAutomation.count).to eq(1)
+      end
+    end
+  end
+
   context "when trigger is called" do
     before do
       freeze_time Time.zone.parse("2021-06-04 10:00")