Fix schedule hooks firing when timestamp <= now

lib/resque/scheduler/delaying_extensions.rb

@@ -21,19 +21,24 @@ def enqueue_at(timestamp, klass, *args)
       # timestamp has passed. It respects Resque.inline option, by
       # creating the job right away instead of adding to the queue.
       def enqueue_at_with_queue(queue, timestamp, klass, *args)
-        plugin.process_schedule_hooks(klass, *args) do
-          if Resque.inline? || timestamp.to_i <= Time.now.to_i
-            # Just create the job and let resque perform it right away with
-            # inline.  If the class is a custom job class, call self#scheduled
-            # on it. This allows you to do things like
-            # Resque.enqueue_at(timestamp, CustomJobClass, :opt1 => val1).
-            # Otherwise, pass off to Resque.
-            if klass.respond_to?(:scheduled)
-              klass.scheduled(queue, klass.to_s, *args)
-            else
-              Resque.enqueue_to(queue, klass, *args)
-            end
+        if Resque.inline? || timestamp.to_i <= Time.now.to_i
+          # Just create the job and let resque perform it right away with
+          # inline.  If the class is a custom job class, call self#scheduled
+          # on it. This allows you to do things like
+          # Resque.enqueue_at(timestamp, CustomJobClass, :opt1 => val1).
+          # Otherwise, pass off to Resque.
+          #
+          # Note: schedule hooks are NOT called here because the job is not
+          # actually being added to the delayed queue.
+          if klass.respond_to?(:scheduled)
+            klass.scheduled(queue, klass.to_s, *args)
           else
+            Resque.enqueue_to(queue, klass, *args)
+          end
+        else
+          # Job is being added to the delayed queue, so we call the
+          # before_schedule and after_schedule hooks.
+          plugin.process_schedule_hooks(klass, *args) do
             delayed_push(timestamp, job_to_hash_with_queue(queue, klass, args))
           end
         end

test/scheduler_hooks_test.rb

@@ -12,7 +12,7 @@ def config
     }
   end
 
-  # helper to inspected the queue
+  # helper to inspect the queue
   def enqueued
     Resque.redis.lrange("queue:#{SomeRealClass.queue}", 0, -1).map(&JSON.method(:parse))
   end
@@ -52,6 +52,86 @@ def enqueued
                  'job should not be enqueued')
   end
 
+  test 'schedule hooks are not called when timestamp is in the past' do
+    SomeRealClass.expects(:before_schedule_example).never
+    SomeRealClass.expects(:after_schedule_example).never
+
+    past_time = Time.now - 3600
+    Resque.enqueue_at(past_time, SomeRealClass, :foo)
+
+    assert_equal(0, Resque.count_all_scheduled_jobs,
+                 'job should not be in delayed queue')
+    assert_equal(1, Resque.redis.llen("queue:#{SomeRealClass.queue}"),
+                 'job should be in work queue')
+  end
+
+  test 'schedule hooks are not called when timestamp equals now' do
+    Timecop.freeze do
+      SomeRealClass.expects(:before_schedule_example).never
+      SomeRealClass.expects(:after_schedule_example).never
+
+      Resque.enqueue_at(Time.now, SomeRealClass, :foo)
+
+      assert_equal(0, Resque.count_all_scheduled_jobs,
+                   'job should not be in delayed queue')
+      assert_equal(1, Resque.redis.llen("queue:#{SomeRealClass.queue}"),
+                   'job should be in work queue')
+    end
+  end
+
+  test 'schedule hooks are called when timestamp is in the future' do
+    future_time = Time.now + 3600
+    SomeRealClass.expects(:before_schedule_example).with(:foo)
+    SomeRealClass.expects(:after_schedule_example).with(:foo)
+
+    Resque.enqueue_at(future_time, SomeRealClass, :foo)
+
+    assert_equal(1, Resque.count_all_scheduled_jobs,
+                 'job should be in delayed queue')
+    assert_equal(0, Resque.redis.llen("queue:#{SomeRealClass.queue}"),
+                 'job should not be in work queue')
+  end
+
+  test 'resque enqueue hooks are called when timestamp is in the past' do
+    SomeJobWithResqueHooks.expects(:before_enqueue_example).with(:foo)
+    SomeJobWithResqueHooks.expects(:after_enqueue_example).with(:foo)
+
+    past_time = Time.now - 3600
+    Resque.enqueue_at(past_time, SomeJobWithResqueHooks, :foo)
+
+    assert_equal(0, Resque.count_all_scheduled_jobs,
+                 'job should not be in delayed queue')
+    assert_equal(1, Resque.redis.llen("queue:#{SomeJobWithResqueHooks.queue}"),
+                 'job should be in work queue')
+  end
+
+  test 'resque enqueue hooks are called when timestamp equals now' do
+    Timecop.freeze do
+      SomeJobWithResqueHooks.expects(:before_enqueue_example).with(:foo)
+      SomeJobWithResqueHooks.expects(:after_enqueue_example).with(:foo)
+
+      Resque.enqueue_at(Time.now, SomeJobWithResqueHooks, :foo)
+
+      assert_equal(0, Resque.count_all_scheduled_jobs,
+                   'job should not be in delayed queue')
+      assert_equal(1, Resque.redis.llen("queue:#{SomeJobWithResqueHooks.queue}"),
+                   'job should be in work queue')
+    end
+  end
+
+  test 'resque enqueue hooks are not called when timestamp is in the future' do
+    SomeJobWithResqueHooks.expects(:before_enqueue_example).never
+    SomeJobWithResqueHooks.expects(:after_enqueue_example).never
+
+    future_time = Time.now + 3600
+    Resque.enqueue_at(future_time, SomeJobWithResqueHooks, :foo)
+
+    assert_equal(1, Resque.count_all_scheduled_jobs,
+                 'job should be in delayed queue')
+    assert_equal(0, Resque.redis.llen("queue:#{SomeJobWithResqueHooks.queue}"),
+                 'job should not be in work queue')
+  end
+
   test 'default failure hooks are called when enqueueing a job fails' do
     e = RuntimeError.new('custom error')
     Resque::Scheduler.expects(:enqueue_from_config).raises(e)