Improve trigger_schedule.rb

4 files changed, 91 insertions, 17 deletions

diff --git a/app/models/ci/trigger_schedule.rb b/app/models/ci/trigger_schedule.rb
index d18dbea284e..256e609f0d1 100644
--- a/app/models/ci/trigger_schedule.rb
+++ b/app/models/ci/trigger_schedule.rb
@@ -15,11 +15,16 @@ module Ci
     validates :cron_timezone, cron_timezone: true, presence: { unless: :importing? }
     validates :ref, presence: { unless: :importing? }
 
-    after_create :schedule_next_run!
+    before_save :set_next_run_at
+
+    def set_next_run_at
+      self.next_run_at = Gitlab::Ci::CronParser.new(cron, cron_timezone).next_time_from(Time.now)
+    end
 
     def schedule_next_run!
-      next_time = Gitlab::Ci::CronParser.new(cron, cron_timezone).next_time_from(Time.now)
-      update!(next_run_at: next_time) if next_time.present?
+      save! # with set_next_run_at
+    rescue ActiveRecord::RecordInvalid => invalid
+      update_attribute(:next_run_at, nil) # update without validation
     end
   end
 end
diff --git a/spec/factories/ci/trigger_schedules.rb b/spec/factories/ci/trigger_schedules.rb
index 49d2b29727f..315bce16995 100644
--- a/spec/factories/ci/trigger_schedules.rb
+++ b/spec/factories/ci/trigger_schedules.rb
@@ -8,12 +8,6 @@ FactoryGirl.define do
       trigger_schedule.update!(project: trigger_schedule.trigger.project)
     end
 
-    trait :force_triggable do
-      after(:create) do |trigger_schedule, evaluator|
-        trigger_schedule.update!(next_run_at: trigger_schedule.next_run_at - 1.year)
-      end
-    end
-
     trait :nightly do
       cron '0 1 * * *'
       cron_timezone Gitlab::Ci::CronParser::VALID_SYNTAX_SAMPLE_TIME_ZONE
diff --git a/spec/models/ci/trigger_schedule_spec.rb b/spec/models/ci/trigger_schedule_spec.rb
index 22c0790688c..75d21541cee 100644
--- a/spec/models/ci/trigger_schedule_spec.rb
+++ b/spec/models/ci/trigger_schedule_spec.rb
@@ -5,16 +5,72 @@ describe Ci::TriggerSchedule, models: true do
   it { is_expected.to belong_to(:trigger) }
   it { is_expected.to respond_to(:ref) }
 
+  describe '#set_next_run_at' do
+    context 'when creates new TriggerSchedule' do
+      before do
+        trigger_schedule = create(:ci_trigger_schedule, :nightly)
+        @expected_next_run_at = Gitlab::Ci::CronParser.new(trigger_schedule.cron, trigger_schedule.cron_timezone)
+                                                      .next_time_from(Time.now)
+      end
+
+      it 'updates next_run_at automatically' do
+        expect(Ci::TriggerSchedule.last.next_run_at).to eq(@expected_next_run_at)
+      end
+    end
+
+    context 'when updates cron of exsisted TriggerSchedule' do
+      before do
+        trigger_schedule = create(:ci_trigger_schedule, :nightly)
+        new_cron = '0 0 1 1 *'
+        trigger_schedule.update!(cron: new_cron) # Subject
+        @expected_next_run_at = Gitlab::Ci::CronParser.new(new_cron, trigger_schedule.cron_timezone)
+                                                      .next_time_from(Time.now)
+      end
+
+      it 'updates next_run_at automatically' do
+        expect(Ci::TriggerSchedule.last.next_run_at).to eq(@expected_next_run_at)
+      end
+    end
+  end
+
   describe '#schedule_next_run!' do
-    let(:trigger_schedule) { create(:ci_trigger_schedule, :nightly) }
-    let(:next_time) { Gitlab::Ci::CronParser.new(trigger_schedule.cron, trigger_schedule.cron_timezone).next_time_from(Time.now) }
+    context 'when reschedules after 10 days from now' do
+      before do
+        trigger_schedule = create(:ci_trigger_schedule, :nightly)
+        time_future = Time.now + 10.days
+        allow(Time).to receive(:now).and_return(time_future)
+        trigger_schedule.schedule_next_run! # Subject
+        @expected_next_run_at = Gitlab::Ci::CronParser.new(trigger_schedule.cron, trigger_schedule.cron_timezone)
+                                                      .next_time_from(time_future)
+      end
 
-    before do
-      trigger_schedule.schedule_next_run!
+      it 'points to proper next_run_at' do
+        expect(Ci::TriggerSchedule.last.next_run_at).to eq(@expected_next_run_at)
+      end
     end
 
-    it 'updates next_run_at' do
-      expect(Ci::TriggerSchedule.last.next_run_at).to eq(next_time)
+    context 'when cron is invalid' do
+      before do
+        trigger_schedule = create(:ci_trigger_schedule, :nightly)
+        trigger_schedule.cron = 'Invalid-cron'
+        trigger_schedule.schedule_next_run! # Subject
+      end
+
+      it 'sets nil to next_run_at' do
+        expect(Ci::TriggerSchedule.last.next_run_at).to be_nil
+      end
+    end
+
+    context 'when cron_timezone is invalid' do
+      before do
+        trigger_schedule = create(:ci_trigger_schedule, :nightly)
+        trigger_schedule.cron_timezone = 'Invalid-cron_timezone'
+        trigger_schedule.schedule_next_run! # Subject
+      end
+
+      it 'sets nil to next_run_at' do
+        expect(Ci::TriggerSchedule.last.next_run_at).to be_nil
+      end
     end
   end
 end
diff --git a/spec/workers/trigger_schedule_worker_spec.rb b/spec/workers/trigger_schedule_worker_spec.rb
index 3997369489f..c48f1406c34 100644
--- a/spec/workers/trigger_schedule_worker_spec.rb
+++ b/spec/workers/trigger_schedule_worker_spec.rb
@@ -8,10 +8,12 @@ describe TriggerScheduleWorker do
   end
 
   context 'when there is a scheduled trigger within next_run_at' do
-    let!(:trigger_schedule) { create(:ci_trigger_schedule, :nightly, :force_triggable) }
-    let(:next_time) { Gitlab::Ci::CronParser.new(trigger_schedule.cron, trigger_schedule.cron_timezone).next_time_from(Time.now) }
+    let!(:trigger_schedule) { create(:ci_trigger_schedule, :nightly) }
+    let(:next_time) { Gitlab::Ci::CronParser.new(trigger_schedule.cron, trigger_schedule.cron_timezone).next_time_from(@time_future) }
 
     before do
+      @time_future = Time.now + 10.days
+      allow(Time).to receive(:now).and_return(@time_future)
       worker.perform
     end
 
@@ -43,4 +45,21 @@ describe TriggerScheduleWorker do
       expect(trigger_schedule.next_run_at).to eq(Ci::TriggerSchedule.last.next_run_at)
     end
   end
+
+  context 'when next_run_at is nil' do
+    let!(:trigger_schedule) { create(:ci_trigger_schedule, :nightly) }
+
+    before do
+      trigger_schedule.update_attribute(:next_run_at, nil)
+      worker.perform
+    end
+
+    it 'does not create a new pipeline' do
+      expect(Ci::Pipeline.count).to eq(0)
+    end
+
+    it 'does not update next_run_at' do
+      expect(trigger_schedule.next_run_at).to eq(Ci::TriggerSchedule.last.next_run_at)
+    end
+  end
 end