Fixed legacy purge worker. Changed conversion worker to run only once (i.e. not retry errors) and to always send the callback if defined. Changed callback to POST variables, rather than sending JSON

Author: Joe Blackman

Gemfile

@@ -18,4 +18,5 @@ gem 'rspec'
 gem 'rack-test'
 gem 'simplecov'
 gem 'byebug'
+gem 'timecop'
 #gem 'api-config', git: 'http://code.ifad.org/api-config'

Gemfile.lock

@@ -76,6 +76,7 @@ GEM
       eventmachine (~> 1.0)
       rack (~> 1.0)
     tilt (1.4.1)
+    timecop (0.7.1)
     timers (4.0.1)
       hitimes
     unicorn (4.3.1)
@@ -103,5 +104,6 @@ DEPENDENCIES
   simplecov
   sinatra
   thin
+  timecop
   unicorn
   wkhtmltopdf

README.md

@@ -271,3 +271,17 @@ Response:
       Content-Type: application/pdf; charset=binary
       
       ... PDF document body ...
+
+## Callbacks
+
+When a document conversion is completed, an attempt will be made to POST a callback to the URL specified when the conversion was attempted. The callback will be a normal form post, sending these values:
+
+    status - the result of the conversion, 200 for success, 400+ for failure
+    description - the outcome of the conversion, e.g. "Document converted"
+    app         - the application name
+    doc_id      - the ID of the document
+    version     - the version of the document that was converted
+    action      - the conversion action performed
+    path        - a path to the converted file (you will have to tack the Colore URL base onto this)
+
+

lib/sidekiq_workers.rb

@@ -2,6 +2,7 @@
 # Sidekiq workers for the Colore system.
 #
 #
+require 'rest_client'
 require 'sidekiq'
 require 'sidetiq'
 
@@ -10,7 +11,7 @@ module Sidekiq
     # This worker converts a document file to a new format and stores it.
     class ConversionWorker
       include ::Sidekiq::Worker
-      sidekiq_options queue: :colore, retry: 5, backtrace: true
+      sidekiq_options queue: :colore, retry: false
 
       # Converts a document file to a new format. The converted file will be stored in
       # the document version directory. If the callback_url is specified, the [CallbackWorker]
@@ -23,9 +24,18 @@ class ConversionWorker
       def perform doc_key_str, version, filename, action, callback_url=nil
         doc_key = DocKey.parse doc_key_str
         new_filename = Converter.new.convert doc_key, version, filename, action
-        CallbackWorker.perform_async doc_key_str, version, action, new_filename, callback_url if callback_url
+        status = 200
+        message = "Document converted"
       rescue Heathen::TaskNotFound => e
         logger.warn "#{e.message}, will not attempt to re-process this request"
+        status = 400
+        message = e.message
+      rescue StandardError => e
+        logger.warn "#{e.message}, will not attempt to re-process this request"
+        status = 500
+        message = e.message
+      ensure
+        CallbackWorker.perform_async doc_key_str, version, action, new_filename, callback_url, status, message if callback_url
       end
     end
 
@@ -39,20 +49,22 @@ class CallbackWorker
       # @param version [String] the file version
       # @param action [String] the conversion to perform
       # @param filename [String] the converted file name
-      # @param callback_url [Stringoptional callback URL
-      def perform doc_key_str, version, action, new_filename, callback_url
+      # @param callback_url [String] callback URL
+      # @param status [Integer) status code to send in callback
+      # @param message [String] description text to send in callback
+      def perform doc_key_str, version, action, new_filename, callback_url, status, description
         doc_key = DocKey.parse doc_key_str
         doc = Document.load C_.storage_directory, doc_key
         rsp_hash = {
-          status: 200,
-          description: "Document converted",
+          status: status.to_i,
+          description: description,
           app: doc_key.app,
           doc_id: doc_key.doc_id,
           version: version,
           action: action,
-          path: doc.file_path(version,new_filename),
+          path: (doc.file_path(version,new_filename) if status < 300),
         }
-        RestClient.post callback_url, JSON.pretty_generate(rsp_hash), content_type: :json
+        RestClient.post callback_url, rsp_hash
       end
     end
 
@@ -69,11 +81,12 @@ class LegacyPurgeWorker
 
       # Looks for old legacy docs and deletes them
       def perform
-        purge_seconds = (C_.legacy_purge_days || 1).to_i * 86400.0
-        LegacyConverter.new.legacy_dir.each_entry do |file|
-          next if file.directory?
-          if Time.now - file.ctime > purge_seconds
-            file.unlink
+        purge_seconds = (C_.legacy_purge_days || 1).to_f * 86400.0
+        dir = LegacyConverter.new.legacy_dir
+        dir.each_entry do |file|
+          next if (dir+file).directory?
+          if Time.now - (dir+file).ctime > purge_seconds
+            (dir+file).unlink
             logger.debug "Deleted old legacy file: #{file}"
           end
         end

spec/helpers/storage.rb

@@ -8,6 +8,7 @@ def setup_storage
   FileUtils.rm_rf tmp_storage_dir
   FileUtils.mkdir_p tmp_storage_dir
   FileUtils.cp_r fixture('app'), tmp_storage_dir
+  FileUtils.cp_r fixture('legacy'), tmp_storage_dir
 end
 
 def delete_storage

spec/integration/callback.rb

@@ -0,0 +1,13 @@
+#!/usr/bin/env ruby
+#
+# Simple sinatra app to receive and display callbacks
+#
+require 'pp'
+require 'sinatra'
+
+set :port, 9230
+
+post '/callback' do
+  puts "Received callback"
+  pp params
+end

spec/lib/sidekiq_workers_spec.rb

@@ -19,7 +19,13 @@
 
     it 'gives up on Heathen::TaskNotFound' do
       allow(@mock_converter).to receive(:convert) { raise Heathen::TaskNotFound.new('foo','bar') }
-      expect(Colore::Sidekiq::CallbackWorker).not_to receive(:perform_async)
+      expect(Colore::Sidekiq::CallbackWorker).to receive(:perform_async) {}
+      described_class.new.perform doc_key.to_s, 'current', 'arglebargle.docx', 'pdf', callback_url
+    end
+
+    it 'gives up on other errors' do
+      allow(@mock_converter).to receive(:convert) { raise 'arglebargle' }
+      expect(Colore::Sidekiq::CallbackWorker).to receive(:perform_async) {}
       described_class.new.perform doc_key.to_s, 'current', 'arglebargle.docx', 'pdf', callback_url
     end
   end
@@ -37,8 +43,39 @@
   end
   context '#perform' do
     it 'runs' do
-      expect(RestClient).to receive(:post).with(callback_url,String,Hash)
-      described_class.new.perform doc_key.to_s, 'current', 'arglebargle.docx', 'pdf', callback_url
+      expect(RestClient).to receive(:post).with(callback_url,Hash)
+      described_class.new.perform doc_key.to_s, 'current', 'arglebargle.docx', 'pdf', callback_url, 250, 'foobar'
+    end
+  end
+end
+
+describe Colore::Sidekiq::LegacyPurgeWorker do
+  before do
+    setup_storage
+    allow(Colore::C_).to receive(:storage_directory) { tmp_storage_dir }
+    allow(Colore::C_).to receive(:legacy_purge_days) { 2 }
+  end
+  after do
+    delete_storage
+  end
+  context '#perform' do
+    it 'runs' do
+      dir = Colore::LegacyConverter.new.legacy_dir
+      file1 = dir + 'file1.tiff'
+      file2 = dir + 'file2.tiff'
+      file1.open('w') { |f| f.write 'foobar' }
+      file2.open('w') { |f| f.write 'foobar' }
+      described_class.new.perform
+      expect(file1.file?).to eq true
+      expect(file2.file?).to eq true
+      Timecop.freeze(Date.today + 1)
+      described_class.new.perform
+      expect(file1.file?).to eq true
+      expect(file2.file?).to eq true
+      Timecop.freeze(Date.today + 3)
+      described_class.new.perform
+      expect(file1.file?).to eq false
+      expect(file2.file?).to eq false
     end
   end
 end

spec/spec_helper.rb

@@ -4,6 +4,7 @@
 require 'byebug'
 require 'rack/test'
 require 'simplecov'
+require 'timecop'
 
 SimpleCov.start