Add validation to Ci::TriggerSchedule (Halfway)

5 files changed, 72 insertions, 29 deletions

diff --git a/app/models/ci/trigger_schedule.rb b/app/models/ci/trigger_schedule.rb
index 7efaa228a93..1b3b73971bb 100644
--- a/app/models/ci/trigger_schedule.rb
+++ b/app/models/ci/trigger_schedule.rb
@@ -7,15 +7,42 @@ module Ci
     belongs_to :project
     belongs_to :trigger
 
+    validates :trigger, presence: true
+    validates :cron, presence: true
+    validates :cron_time_zone, presence: true
+    validate :check_cron
+    validate :check_ref
+
+    after_create :schedule_next_run!
+
     def schedule_next_run!
       next_time = Ci::CronParser.new(cron, cron_time_zone).next_time_from_now
       if next_time.present?
-        update_attributes(next_run_at: next_time)
+        update!(next_run_at: next_time)
       end
     end
 
-    # def update_last_run!
-    #   update_attributes(last_run_at: Time.now)
-    # end
+    private
+
+    def check_cron
+      cron_parser = Ci::CronParser.new(cron, cron_time_zone)
+      is_valid_cron, is_valid_cron_time_zone = cron_parser.validation
+
+      if !is_valid_cron
+        self.errors.add(:cron, " is invalid syntax")
+      elsif !is_valid_cron_time_zone
+        self.errors.add(:cron_time_zone, " is invalid timezone")
+      elsif (cron_parser.next_time_from_now - Time.now).abs < 1.hour
+        self.errors.add(:cron, " can not be less than 1 hour")
+      end
+    end
+
+    def check_ref
+      if !trigger.ref.present?
+        self.errors.add(:ref, " is empty")
+      elsif trigger.project.repository.ref_exists?(trigger.ref)
+        self.errors.add(:ref, " does not exist")
+      end
+    end
   end
 end
diff --git a/lib/ci/cron_parser.rb b/lib/ci/cron_parser.rb
index 24c4dd676dc..b0ffb48dabc 100644
--- a/lib/ci/cron_parser.rb
+++ b/lib/ci/cron_parser.rb
@@ -6,20 +6,22 @@ module Ci
     end
 
     def next_time_from_now
-      cronLine = try_parse_cron
+      cronLine = try_parse_cron(@cron, @cron_time_zone)
       return nil unless cronLine.present?
       cronLine.next_time
     end
 
-    def valid_syntax?
-      try_parse_cron.present? ? true : false
+    def validation
+      is_valid_cron = try_parse_cron(@cron, 'Europe/Istanbul').present?
+      is_valid_cron_time_zone = try_parse_cron('* * * * *', @cron_time_zone).present?
+      return is_valid_cron, is_valid_cron_time_zone
     end
 
     private
 
-    def try_parse_cron
+    def try_parse_cron(cron, cron_time_zone)
       begin
-        Rufus::Scheduler.parse("#{@cron} #{@cron_time_zone}")
+        Rufus::Scheduler.parse("#{cron} #{cron_time_zone}")
       rescue
         nil
       end
diff --git a/spec/factories/ci/trigger_schedules.rb b/spec/factories/ci/trigger_schedules.rb
index f909e343bf2..0dd397435c1 100644
--- a/spec/factories/ci/trigger_schedules.rb
+++ b/spec/factories/ci/trigger_schedules.rb
@@ -4,18 +4,20 @@ FactoryGirl.define do
     trigger factory: :ci_trigger
 
     trait :force_triggable do
-      next_run_at Time.now - 1.month
+      after(:create) do |trigger_schedule, evaluator|
+        trigger_schedule.next_run_at -= 1.month
+      end
     end
 
     trait :cron_nightly_build do
       cron '0 1 * * *'
       cron_time_zone 'Europe/Istanbul'
-      next_run_at do # TODO: Use CronParser
-        time = Time.now.in_time_zone(cron_time_zone)
-        time = time + 1.day if time.hour > 1
-        time = time.change(sec: 0, min: 0, hour: 1)
-        time
-      end
+      # next_run_at do # TODO: Use CronParser
+      #   time = Time.now.in_time_zone(cron_time_zone)
+      #   time = time + 1.day if time.hour > 1
+      #   time = time.change(sec: 0, min: 0, hour: 1)
+      #   time
+      # end
     end
 
     trait :cron_weekly_build do
diff --git a/spec/factories/ci/triggers.rb b/spec/factories/ci/triggers.rb
index 1feaa9b9fa1..d38800b58f7 100644
--- a/spec/factories/ci/triggers.rb
+++ b/spec/factories/ci/triggers.rb
@@ -1,7 +1,7 @@
 FactoryGirl.define do
   factory :ci_trigger_without_token, class: Ci::Trigger do
     factory :ci_trigger do
-      token { SecureRandom.hex(10) }
+      token { SecureRandom.hex(15) }
     end
   end
 end
diff --git a/spec/models/ci/trigger_schedule_spec.rb b/spec/models/ci/trigger_schedule_spec.rb
index 14b8530a65b..d8b6bd93954 100644
--- a/spec/models/ci/trigger_schedule_spec.rb
+++ b/spec/models/ci/trigger_schedule_spec.rb
@@ -1,29 +1,41 @@
 require 'spec_helper'
 
 describe Ci::TriggerSchedule, models: true do
+  let(:user) { create(:user) }
+  let(:project) { create(:project) }
+  let(:trigger) { create(:ci_trigger, owner: user, project: project, ref: 'master') }
 
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:trigger) }
   end
 
-  describe '#schedule_next_run!' do
-    subject { trigger_schedule.schedule_next_run! }
+  describe 'validation' do
+    let(:trigger_schedule) { create(:ci_trigger_schedule, :cron_nightly_build, trigger: trigger) }
 
-    let(:trigger_schedule) { create(:ci_trigger_schedule, :cron_nightly_build, next_run_at: nil) }
+    it { expect(trigger_schedule).to validate_presence_of(:trigger) }
+    it { is_expected.to validate_presence_of(:cron) }
+    it { is_expected.to validate_presence_of(:cron_time_zone) }
 
-    it 'updates next_run_at' do
-      is_expected.not_to be_nil
+    it '#check_cron' do
+      subject.cron = 'Hack'
+      subject.valid?
+      subject.errors[:screen_name].to include(' is invalid syntax')
+    end
+
+    it '#check_ref' do
     end
   end
 
-  # describe '#update_last_run!' do
-  #   subject { scheduled_trigger.update_last_run! }
+  describe '#schedule_next_run!' do
+    let(:trigger_schedule) { create(:ci_trigger_schedule, :cron_nightly_build, next_run_at: nil, trigger: trigger) }
 
-  #   let(:scheduled_trigger) { create(:ci_scheduled_trigger, :cron_nightly_build, last_run_at: nil) }
+    before do
+      trigger_schedule.schedule_next_run!
+    end
 
-  #   it 'updates last_run_at' do
-  #     is_expected.not_to be_nil
-  #   end
-  # end
+    it 'updates next_run_at' do
+      expect(Ci::TriggerSchedule.last.next_run_at).not_to be_nil
+    end
+  end
 end