Merge pull request #76 from eisuke/fix_next_schedule

Resolve next scheduling ploblems

app/assets/javascripts/kuroko2/suspend_schedule_handler.js

@@ -0,0 +1,9 @@
+$(function(){
+  $("#new_job_suspend_schedule").
+    on("ajax:success", function(e, data, status, xhr){
+      $("#suspend-cron-error").hide();
+    }).
+    on("ajax:error", function(e, xhr, status, error) {
+      $("#suspend-cron-error").show().html("<p>Error: " + xhr.responseJSON.join(' ') + "</p>");
+    });
+});

app/assets/stylesheets/kuroko2/job_definitions.scss

@@ -23,3 +23,8 @@
     color: #888;
   }
 }
+
+#suspend-cron-error {
+  display: none;
+  color: #843534;
+}

app/controllers/kuroko2/job_suspend_schedules_controller.rb

@@ -13,7 +13,7 @@ def create
     if suspend_schedule.valid?
       render json: suspend_schedule, status: :created
     else
-      render json: suspend_schedule, status: :bad_request
+      render json: suspend_schedule.errors.full_messages, status: :bad_request
     end
   end
 

app/models/kuroko2/job_schedule.rb

@@ -15,17 +15,16 @@ class Kuroko2::JobSchedule < Kuroko2::ApplicationRecord
     (?:[0-6]|(?:(?:[0-6]\-[0-6]|\*)(?:\/[0-6])?))(?:,(?:[0-6]|(?:(?:[0-6]\-[0-6]|\*)(?:\/[0-6])?)))*
   \z/x
 
+  CHRONO_SCHEDULE_METHODS = %i[minutes hours days months wdays]
+
   validates :cron, format: { with: CRON_FORMAT }, uniqueness: { scope: :job_definition_id }
   validate :validate_cron_schedule
 
   def next(now = Time.current)
-    if 1.month.ago(now).future?
-      Kuroko2.logger.warn("Exceeds the time of criteria #{now}. (Up to 1 month since)")
-      return
-    end
+    return if suspended_all?
 
     next_time = Chrono::Iterator.new(self.cron, now: now).next
-    suspend_times = suspend_times(now, next_time)
+    suspend_times = suspend_times(next_time, next_time)
 
     if suspend_times.include?(next_time)
       self.next(next_time)
@@ -51,7 +50,7 @@ def scheduled_times(time_from, time_to)
   end
 
   def suspend_times(time_from, time_to)
-    if job_definition && job_definition.job_suspend_schedules.present?
+    if has_suspend_schedules?
       job_definition.job_suspend_schedules.
         map { |schedule| schedule.suspend_times(time_from, time_to) }.flatten.uniq
     else
@@ -61,6 +60,43 @@ def suspend_times(time_from, time_to)
 
   private
 
+  def suspended_all?
+    return false unless has_suspend_schedules?
+
+    launch_schedule = Chrono::Schedule.new(cron)
+    schedule = CHRONO_SCHEDULE_METHODS.each_with_object({}) do |method, h|
+      h[method] = launch_schedule.public_send(method)
+    end
+
+    job_definition.job_suspend_schedules.each do |suspend_schedule_model|
+      suspend_schedule = Chrono::Schedule.new(suspend_schedule_model.cron)
+      CHRONO_SCHEDULE_METHODS.each do |method|
+        schedule[method] -= suspend_schedule.public_send(method)
+      end
+
+      # https://linux.die.net/man/5/crontab
+      # > Note: The day of a command's execution can be specified by two fields
+      # >  day of month, and day of week. If both fields are restricted (ie, aren't *),
+      # >  the command will be run when either field matches the current time.
+      # >  For example, "30 4 1,15 * 5" would cause a command to be run at 4:30 am
+      # >  on the 1st and 15th of each month, plus every Friday.
+      if suspend_schedule.wdays? && suspend_schedule.days?
+        case
+        when launch_schedule.wdays? && !launch_schedule.days?
+          schedule[:days] = []
+        when !launch_schedule.wdays? && launch_schedule.days?
+          schedule[:wdays] = []
+        end
+      end
+    end
+
+    schedule.values.all?(&:empty?)
+  end
+
+  def has_suspend_schedules?
+    job_definition && !job_definition.job_suspend_schedules.empty?
+  end
+
   def validate_cron_schedule
     if CRON_FORMAT === cron
       self.next

app/models/kuroko2/job_suspend_schedule.rb

@@ -26,9 +26,37 @@ def suspend_times(time_from, time_to)
 
   def validate_cron_schedule
     if Kuroko2::JobSchedule::CRON_FORMAT === cron
-      Chrono::Iterator.new(self.cron).next
+      suspend_schedule = Chrono::Schedule.new(cron)
+      if job_definition.job_schedules.empty?
+        errors.add(:cron, "needs job schedules")
+      else
+        schedule = job_definition.job_schedules.each_with_object({}) do |launch_schedule_model, h|
+          launch_schedule = Chrono::Schedule.new(launch_schedule_model.cron)
+          Kuroko2::JobSchedule::CHRONO_SCHEDULE_METHODS.each do |method|
+            h[method] ||= []
+
+            suspend_schedule_list = suspend_schedule.public_send(method)
+            # https://linux.die.net/man/5/crontab
+            # > Note: The day of a command's execution can be specified by two fields
+            # >  day of month, and day of week. If both fields are restricted (ie, aren't *),
+            # >  the command will be run when either field matches the current time.
+            # >  For example, "30 4 1,15 * 5" would cause a command to be run at 4:30 am
+            # >  on the 1st and 15th of each month, plus every Friday.
+            if launch_schedule.wdays? && launch_schedule.days?
+              if (method == :wdays && suspend_schedule.wdays? && !suspend_schedule.days?) || (method == :days && !suspend_schedule.wdays? && suspend_schedule.days?)
+                suspend_schedule_list = []
+              end
+            end
+
+            h[method] |= launch_schedule.public_send(method) - suspend_schedule_list
+          end
+        end
+
+        if schedule.values.all?(&:empty?)
+          errors.add(:cron, "suspends all launched schedules")
+        end
+      end
     end
-    nil
   rescue Chrono::Fields::Base::InvalidField => e
     errors.add(:cron, "has invalid field: #{e.message}")
   end

app/views/kuroko2/job_definitions/_list.html.slim

@@ -13,9 +13,12 @@
         tr
           td= definition.id
           td.no-decorate= link_to definition.name, definition
-          - if !definition.suspended? && definition.job_schedules.present?
+          - if !definition.suspended? && !definition.job_schedules.empty?
             - next_job_schedule = definition.job_schedules.map(&:next).min
-            td data-value=next_job_schedule.to_i = l(next_job_schedule, format: :short)
+            - if next_job_schedule
+              td data-value=next_job_schedule.to_i = l(next_job_schedule, format: :short)
+            - else
+              td data-value="9999999999" --
           - else
             td data-value="9999999999" --
           td= labeled_status(definition.job_instances.take)

app/views/kuroko2/job_suspend_schedules/index.html.slim

@@ -21,7 +21,10 @@
   = form_for([@definition, @suspend_schedule], remote: true) do |form|
     .box-body
       #suspend-cron-field.row.form-group
+        .col-md-12
+          div id="suspend-cron-error"
         .col-md-8
           = form.text_field :cron, class: 'form-control script-input', placeholder: '0 9 * * *'
+          = javascript_include_tag 'kuroko2/suspend_schedule_handler'
         .col-md-4
           = form.submit 'Add Suspend Schedule', class: 'btn btn-default btn-block'

config/initializers/assets.rb

@@ -6,4 +6,4 @@
 # Precompile additional assets.
 # application.js, application.css, and all non-JS/CSS in app/assets folder are already added.
 # Rails.application.config.assets.precompile += %w( search.js )
-Rails.application.config.assets.precompile += %w( kuroko2/instance_linker.js timeline/* network/* )
+Rails.application.config.assets.precompile += %w( kuroko2/instance_linker.js kuroko2/suspend_schedule_handler.js timeline/* network/* )

spec/controllers/job_suspend_schedules_controller_spec.rb

@@ -4,36 +4,34 @@
   routes { Kuroko2::Engine.routes }
 
   before { sign_in }
-
-  let(:schedules) { create_list(:job_suspend_schedule, 1) }
-  let(:definition) { create(:job_definition, job_suspend_schedules: schedules) }
+  let(:schedule) { create(:job_schedule, cron: '0 10 * * *') }
+  let(:suspend_schedule) { create(:job_suspend_schedule, job_definition: schedule.job_definition, cron: '0 10 * * 1') }
 
   describe '#index' do
     it do
-      get :index, params: { job_definition_id: definition.id }
+      get :index, params: { job_definition_id: suspend_schedule.job_definition.id }
 
       expect(response).to have_http_status(:ok)
       expect(assigns(:suspend_schedule)).to be_new_record
-      expect(assigns(:suspend_schedules)).to eq schedules
+      expect(assigns(:suspend_schedules)).to eq [suspend_schedule]
     end
   end
 
   describe '#create' do
     it do
-      post :create, params: { job_definition_id: definition.id, job_suspend_schedule: { cron: '* * * * *' } }
-
+      post :create, params: { job_definition_id: suspend_schedule.job_definition.id, job_suspend_schedule: { cron: '0 10 * * 0' } }
       expect(response).to have_http_status(:created)
     end
   end
 
   describe '#destroy' do
 
     it do
-      delete :destroy, params: { job_definition_id: definition.id, id: schedules.first.id }
-      definition.reload
+      delete :destroy, params: { job_definition_id: suspend_schedule.job_definition.id, id: suspend_schedule.id }
+      schedule.job_definition.reload
 
       expect(response).to have_http_status(:ok)
-      expect(definition.job_schedules.size).to be 0
+      expect(schedule.job_definition.job_suspend_schedules.size).to be 0
     end
   end
 end

spec/factories/job_schedule_factory.rb

@@ -6,5 +6,7 @@
 
       "#{n % 60} #{hour} #{day} * *"
     end
+
+    job_definition
   end
 end

spec/factories/job_suspend_schedule_factory.rb

@@ -1,8 +1,5 @@
 FactoryGirl.define do
   factory :job_suspend_schedule, class: Kuroko2::JobSuspendSchedule do
-    sequence(:cron) do |n|
-      hour = n / 60 >= 1 ? (n / 60) : '*'
-      "#{n % 60} #{hour} * * *"
-    end
+    job_definition
   end
 end

spec/features/job_definition_spec.rb

@@ -64,7 +64,21 @@
 
     fill_in 'job_suspend_schedule_cron', with: '* * * * *'
     click_on 'Add Suspend Schedule'
-    expect(page).to have_selector('#suspend-schedules table tbody tr .log', text: '* * * * *', count: 1)
+    expect(page).to have_content('Cron needs job schedules')
+    expect(page).to have_selector('#suspend-schedules table tbody tr .log', text: '* * * * *', count: 0)
+
+    fill_in 'job_schedule_cron', with: '* * * * *'
+    click_on 'Add Schedule'
+    expect(page).to have_selector('#schedules table tbody tr .log', text: '* * * * *', count: 1)
+
+    fill_in 'job_suspend_schedule_cron', with: '* * * * *'
+    click_on 'Add Suspend Schedule'
+    expect(page).to have_content('Cron suspends all launched schedules')
+    expect(page).to have_selector('#suspend-schedules table tbody tr .log', text: '* * * * *', count: 0)
+
+    fill_in 'job_suspend_schedule_cron', with: '* 10 * * *'
+    click_on 'Add Suspend Schedule'
+    expect(page).to have_selector('#suspend-schedules table tbody tr .log', text: '* 10 * * *', count: 1)
 
     within '#suspend-schedules' do
       click_on 'Delete'

spec/models/job_schedule_spec.rb

@@ -28,7 +28,7 @@
 
     context 'With suspend_schelule' do
       before do
-        create(:job_suspend_schedule, job_definition: definition, cron: '0-29 10 * * *')
+        create(:job_suspend_schedule, job_definition: schedule.job_definition, cron: '0-29 10 * * *')
       end
 
       it 'skips suspend time range' do
@@ -38,14 +38,63 @@
 
     context 'When suspend schelules covers all schedule' do
       before do
-        create(:job_suspend_schedule, job_definition: definition, cron: '0-50 10 * * *')
+        suspend_schedule = create(:job_suspend_schedule, job_definition: schedule.job_definition, cron: '50 10 * * *')
+        suspend_schedule.update_column(:cron, '0-50 10 * * *')
       end
 
       it 'returns nil' do
         expect(schedule.next(time)).to be_nil
       end
     end
 
+    context 'When schedules suspended long time ' do
+      let(:time) { Time.new(2016, 1, 2, 10, 0) }
+      let(:cron) { '0 10 1-7 * *' }
+
+      before do
+        create(:job_suspend_schedule, job_definition: schedule.job_definition, cron: '* * * * 0-5')
+      end
+
+      it 'returns next schedule' do
+        expect(schedule.next(time)).to eq(Time.new(2016, 2, 6, 10, 0))
+      end
+    end
+
+    context 'When suspended schedule has wdays and days' do
+      let(:time) { Time.new(2016, 1, 2, 10, 0) }
+      before do
+        create(:job_suspend_schedule, job_definition: schedule.job_definition, cron: '* * 1 * 0') # suspend every sunday or first day of month
+      end
+
+      context 'If the schedule has days only' do
+        let(:cron) { '0 10 1 * *' }
+        it 'returns nil' do
+          expect(schedule.next(time)).to be_nil
+        end
+      end
+
+      context 'If the schedule has wdays only' do
+        let(:cron) { '0 10 * * 0' }
+        it 'returns nil' do
+          expect(schedule.next(time)).to be_nil
+        end
+      end
+
+      context 'If the schedule has wdays and days' do
+        let(:cron) { '0 10 2 * 0' }
+        it 'returns next schedule' do
+          expect(schedule.next(time)).to eq(Time.new(2016, 2, 2, 10, 0))
+        end
+      end
+
+      context 'If the schedule has wdays and days' do
+        let(:cron) { '0 10 1 * 1' }
+        it 'returns next schedule' do
+          expect(schedule.next(time)).to eq(Time.new(2016, 1, 4, 10, 0))
+        end
+      end
+    end
+
     context 'With invalid date' do
       let(:cron) { '* * 31 2 *' }
       let(:time) { Time.new(2016, 2, 28, 10, 0) }

spec/models/job_suspend_schedule_spec.rb

@@ -1,20 +1,54 @@
 require 'rails_helper'
 
 describe Kuroko2::JobSuspendSchedule do
+  let(:launch_schedule) { create(:job_schedule, cron: '* * * * *') }
+
   describe '#valid?' do
     it 'accepts only CRON notation' do
-      expect(Kuroko2::JobSuspendSchedule.new(cron: '* * * * *')).to be_valid
-      expect(Kuroko2::JobSuspendSchedule.new(cron: '1,2-3,*,*/4,5-6/7 1,2-3,*,*/4,5-6/7 1,2-3,*,*/4,5-6/7 1,2-3,*,*/4,5-6/7 1,2-3,*,*/4,1-3/5')).to be_valid
-      expect(Kuroko2::JobSuspendSchedule.new(cron: '* * * *')).not_to be_valid
+      expect(Kuroko2::JobSuspendSchedule.new(cron: '0-30 10 * * *', job_definition: launch_schedule.job_definition)).to be_valid
+      expect(Kuroko2::JobSuspendSchedule.new(cron: '1,2-3,*/4,5-6/7 1,2-3,*,*/4,5-6/7 1,2-3,*,*/4,5-6/7 1,2-3,*,*/4,5-6/7 1,2-3,*,*/4,1-3/5', job_definition: launch_schedule.job_definition)).to be_valid
+      expect(Kuroko2::JobSuspendSchedule.new(cron: '* * * *', job_definition: launch_schedule.job_definition)).not_to be_valid
     end
 
     it 'accepts only valid CRON notation' do
-      expect(Kuroko2::JobSuspendSchedule.new(cron: '5-0 * * * *')).not_to be_valid
+      expect(Kuroko2::JobSuspendSchedule.new(cron: '5-0 * * * *', job_definition: launch_schedule.job_definition)).not_to be_valid
+    end
+
+    context 'when suspend all schedules' do
+      let(:launch_schedule) { create(:job_schedule, cron: '5 10 * * 0') }
+
+      it 'does not accept' do
+        expect(Kuroko2::JobSuspendSchedule.new(cron: '* * * * 0', job_definition: launch_schedule.job_definition)).not_to be_valid
+      end
+
+      context 'when launch schedule has wdays and days' do
+        let(:launch_schedule) { create(:job_schedule, cron: '5 10 1 * 1') }
+
+        it 'does not accept' do
+          expect(Kuroko2::JobSuspendSchedule.new(cron: '* * 1 * 1', job_definition: launch_schedule.job_definition)).not_to be_valid
+        end
+
+        context 'when supend_schedule has only wdays' do
+          let(:launch_schedule) { create(:job_schedule, cron: '5 10 1 * 1') }
+
+          it 'accepts' do
+            expect(Kuroko2::JobSuspendSchedule.new(cron: '* * * * 1', job_definition: launch_schedule.job_definition)).to be_valid
+          end
+        end
+
+        context 'when supend_schedule has only days' do
+          let(:launch_schedule) { create(:job_schedule, cron: '5 10 1 * 1') }
+
+          it 'accepts' do
+            expect(Kuroko2::JobSuspendSchedule.new(cron: '* * 1 * *', job_definition: launch_schedule.job_definition)).to be_valid
+          end
+        end
+      end
     end
   end
 
   describe '#suspend_times' do
-    let(:suspend_schedule) { create(:job_suspend_schedule, cron: '* 10 * * *') }
+    let(:suspend_schedule) { create(:job_suspend_schedule, cron: '* 10 * * *', job_definition: launch_schedule.job_definition) }
     let(:time_from) { Time.new(2016, 1, 1, 10, 0, 0) }
     let(:time_to) { Time.new(2016, 1, 1, 10, 5, 0) }