Refs: #3507_1726@6h Code Review

13 files changed, 148 insertions, 57 deletions

diff --git a/app/models/import.rb b/app/models/import.rb
index d3aa6d21b..c932ecdd9 100644
--- a/app/models/import.rb
+++ b/app/models/import.rb
@@ -6,7 +6,7 @@ class Import < ActiveRecord::Base
   belongs_to :parent, class_name: to_s
 
   extend Enumerize
-  enumerize :status, in: %i(new downloading analyzing pending successful failed running aborted canceled)
+  enumerize :status, in: %i(new pending successful failed running aborted canceled)
 
   validates :file, presence: true
   validates_presence_of :referential, :workbench
diff --git a/app/models/organisation.rb b/app/models/organisation.rb
index 7e571e78d..f697122aa 100644
--- a/app/models/organisation.rb
+++ b/app/models/organisation.rb
@@ -27,10 +27,9 @@ class Organisation < ActiveRecord::Base
     conf = Rails.application.config.try(:stif_portail_api)
     raise 'Rails.application.config.stif_portail_api configuration is not defined' unless conf
 
-    HTTPService.get_resource(
+    HTTPService.get_json_resource(
       host: conf[:url],
       path: '/api/v1/organizations',
-      parse_json: true,
       token: conf[:key])
   end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 1dc5975e1..7fd8b8cee 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -66,10 +66,9 @@ class User < ActiveRecord::Base
     conf = Rails.application.config.try(:stif_portail_api)
     raise 'Rails.application.config.stif_portail_api settings is not defined' unless conf
 
-    HTTPService.get_resource(
+    HTTPService.get_json_resource(
       host: conf[:url],
       path: '/api/v1/users',
-      parse_json: true,
       token: conf[:key])
   end
 
diff --git a/app/services/http_service.rb b/app/services/http_service.rb
index b01e11d6f..694026c4f 100644
--- a/app/services/http_service.rb
+++ b/app/services/http_service.rb
@@ -2,25 +2,29 @@ module HTTPService extend self
 
   Timeout = Faraday::TimeoutError
 
-  def get_resource(host:, path:, token: nil, params: {}, parse_json: false) 
+  def get_resource(host:, path:, token: nil, params: {})
     Faraday.new(url: host) do |c|
       c.headers['Authorization'] = "Token token=#{token.inspect}" if token
       c.adapter Faraday.default_adapter
 
-      resp = c.get path, params
-      if resp.status == 200
-        return parse_json ? JSON.parse(resp.body) : resp.body
-      else
-        raise "Error on api request status : #{resp.status} => #{resp.body}"
-      end
+      return c.get path, params
+    end
+  end
+
+  def get_json_resource(host:, path:, token: nil, params: {})
+    # Stupid Ruby!!!
+    resp = get_resource(host: host, path: path, token: token, params: {}) 
+    if resp.status == 200
+      return JSON.parse(resp.body)
+    else
+      raise "Error on api request status : #{resp.status} => #{resp.body}"
     end
   end
 
   # host: 'http://localhost:3000',
   # path: '/api/v1/netex_imports.json',
-  # resource_name: 'netex_import',
   # token: '13-74009c36638f587c9eafb1ce46e95585',
-  # params: {referential_id: 13, workbench_id: 1},
+  # params: { netex_import: {referential_id: 13, workbench_id: 1}},
   # upload: {file: [StringIO.new('howdy'), 'application/zip', 'greeting']})
   def post_resource(host:, path:, resource_name:, token: nil, params: {}, upload: nil)
     Faraday.new(url: host) do |c|
@@ -35,7 +39,7 @@ module HTTPService extend self
         params.update( name => Faraday::UploadIO.new(value, mime_type, as_name ) )
       end
 
-      c.post path, resource_name => params
+      return c.post path, params
     end
   end
 end
diff --git a/app/services/retry_service.rb b/app/services/retry_service.rb
index 55e2585ef..21b1def36 100644
--- a/app/services/retry_service.rb
+++ b/app/services/retry_service.rb
@@ -1,37 +1,52 @@
+require 'result'
+
 class RetryService
 
   Retry = Class.new(RuntimeError)
 
-  def initialize( delays:, rescue_from: [], &blk )
+  # @param@ delays:
+  # An array of delays that are used to retry after a sleep of the indicated
+  # value in case of failed exceutions.
+  # Once this array is exhausted the executen fails permanently
+  #
+  # @param@ rescue_from:
+  # During execution all the excpetions from this array +plus RetryService::Retry+ are rescued from and
+  # trigger just another retry after a `sleep` as indicated above.
+  #
+  # @param@ block:
+  # This optional code is excuted before each retry, it is passed the result of the failed attempt, thus
+  # an `Exception` and the number of execution already tried.
+  def initialize( delays: [], rescue_from: [], &blk )
     @intervals             = delays
     @registered_exceptions = Array(rescue_from) << Retry
     @failure_callback      = blk
   end
 
+  # @param@ blk:
+  # The code to be executed it will be retried goverened by the `delay` passed into the initializer
+  # as described there in case it fails with one of the predefined exceptions or `RetryService::Retry`
+  #
+  # Eventually it will return a `Result` object.
   def execute &blk
-    status, result = execute_protected blk
-    return [status, result] if status == :ok
+    result = execute_protected blk
+    return result if result.ok?
     @intervals.each_with_index do | interval, retry_count |
-      @failure_callback.try(:call, result, retry_count.succ)
       sleep interval
-      status, result = execute_protected blk
-      return [status, result] if status == :ok
+      @failure_callback.try(:call, result.value, retry_count + 1)
+      result = execute_protected blk
+      return result if result.ok?
     end
-    [status, result]
-  end
-
-  def register_failure_callback &blk
-    @failure_callback = blk
+    result
   end
 
 
   private
 
   def execute_protected blk
-    [:ok, blk.()]
+    Result.ok(blk.())
   rescue Exception => e
     if @registered_exceptions.any?{ |re| e.is_a? re }
-      [:error, e]
+      Result.error(e)
     else
       raise
     end
diff --git a/app/workers/workbench_import_worker.rb b/app/workers/workbench_import_worker.rb
index 3f3d40a00..982d3e0d6 100644
--- a/app/workers/workbench_import_worker.rb
+++ b/app/workers/workbench_import_worker.rb
@@ -18,11 +18,11 @@ class WorkbenchImportWorker
   end
 
   def download
-    logger.warn  "HTTP GET #{import_url}"
+    logger.info  "HTTP GET #{import_url}"
     @zipfile_data = HTTPService.get_resource(
       host: import_host,
       path: import_path,
-      params: {token: @import.token_download})
+      params: {token: @import.token_download}).body
   end
 
   def execute_post eg_name, eg_stream
@@ -30,14 +30,13 @@ class WorkbenchImportWorker
     HTTPService.post_resource(
       host: export_host,
       path: export_path,
-      resource_name: 'netex_import',
       token: token(eg_name),
       params: params,
       upload: {file: [eg_stream, 'application/zip', eg_name]})
   end
 
   def log_failure reason, count
-    logger.info "HTTP POST failed with #{reason}, count = #{count}, response=#{@response}"
+    logger.warn "HTTP POST failed with #{reason}, count = #{count}, response=#{@response}"
   end
 
   def try_again
@@ -61,9 +60,12 @@ class WorkbenchImportWorker
 
   def upload_entry_group key_pair, element_count
     @import.update_attributes( current_step: element_count.succ )
-    retry_service = RetryService.new(delays: RETRY_DELAYS, rescue_from: HTTPService::Timeout, &method(:log_failure)) 
-    status, _ = retry_service.execute(&upload_entry_group_proc(key_pair))
-    raise StopIteration unless status == :ok
+    retry_service = RetryService.new(
+      delays: RETRY_DELAYS,
+      rescue_from: [HTTPService::Timeout],
+      &method(:log_failure)) 
+    status = retry_service.execute(&upload_entry_group_proc(key_pair))
+    raise StopIteration unless status.ok?
   end
 
   def upload_entry_group_proc key_pair
@@ -94,7 +96,7 @@ class WorkbenchImportWorker
     Rails.application.config.front_end_host
   end
   def export_path
-    '/api/v1/netex_imports.json'
+    api_v1_netex_imports_path(format: :json)
   end
   def export_url
     @__export_url__ ||= File.join(export_host, export_path)
@@ -111,6 +113,6 @@ class WorkbenchImportWorker
   end
 
   def params
-    @__params__ ||= { referential_id: @import.referential_id, workbench_id: @import.workbench_id }
+    @__params__ ||= { netex_import: { referential_id: @import.referential_id, workbench_id: @import.workbench_id } }
   end
 end
diff --git a/config/environments/development.rb b/config/environments/development.rb
index 1384d0c00..42523a761 100644
--- a/config/environments/development.rb
+++ b/config/environments/development.rb
@@ -91,7 +91,5 @@ Rails.application.configure do
   # link to validation specification pages
   config.validation_spec = "http://www.chouette.mobi/neptune-validation/v21/"
 
-  # Local zip decompression dir
-  #
   config.i18n.available_locales = [:fr, :en]
 end
diff --git a/hello_world b/hello_world
deleted file mode 100644
index 3b18e512d..000000000
--- a/hello_world
+++ /dev/null
@@ -1 +0,0 @@
-hello world
diff --git a/lib/result.rb b/lib/result.rb
new file mode 100644
index 000000000..96e03d323
--- /dev/null
+++ b/lib/result.rb
@@ -0,0 +1,37 @@
+# A value wrapper adding status information to any value
+# Status can be :ok or :error, we are thusly implementing
+# what is expressed in Elixir/Erlang as result tuples and
+# in Haskell as `Data.Either`
+class Result
+
+  attr_reader :status, :value
+  
+  class << self
+    def ok value
+      make :ok, value
+    end
+    def error value
+      make :error, value
+    end
+
+    def new *args
+      raise NoMethodError, "No default constructor for #{self}"
+    end
+
+    private
+    def make status, value
+      allocate.tap do | o |
+        o.instance_exec do
+          @status = status
+          @value  = value
+        end
+      end
+    end
+  end
+
+  def ok?; status == :ok end
+
+  def == other
+    other.kind_of?(self.class) && other.status == status && other.value == value
+  end
+end
diff --git a/spec/lib/result_spec.rb b/spec/lib/result_spec.rb
new file mode 100644
index 000000000..949de163c
--- /dev/null
+++ b/spec/lib/result_spec.rb
@@ -0,0 +1,20 @@
+RSpec.describe Result do
+
+  context 'is a wrapper of a value' do 
+    it { expect( described_class.ok('hello').value ).to eq('hello') }
+    it { expect( described_class.error('hello').value ).to eq('hello') }
+  end
+
+  context 'it has status information' do 
+    it { expect( described_class.ok('hello') ).to be_ok }
+    it { expect( described_class.ok('hello').status ).to eq(:ok) }
+
+    it { expect( described_class.error('hello') ).not_to be_ok }
+    it { expect( described_class.error('hello').status ).to eq(:error) }
+  end
+  
+  context 'nil is just another value' do
+    it { expect( described_class.ok(nil) ).to be_ok }
+    it { expect( described_class.ok(nil).value ).to be_nil }
+  end
+end
diff --git a/spec/models/import_spec.rb b/spec/models/import_spec.rb
index 424936c58..ee30ce8ba 100644
--- a/spec/models/import_spec.rb
+++ b/spec/models/import_spec.rb
@@ -4,7 +4,7 @@ RSpec.describe Import, :type => :model do
   it { should belong_to(:workbench) }
   it { should belong_to(:parent).class_name(described_class.to_s) }
 
-  it { should enumerize(:status).in("aborted", "analyzing", "canceled", "downloading", "failed", "new", "pending", "running", "successful") }
+  it { should enumerize(:status).in("aborted", "canceled", "failed", "new", "pending", "running", "successful") }
 
   it { should validate_presence_of(:file) }
 end
diff --git a/spec/services/retry_service_spec.rb b/spec/services/retry_service_spec.rb
index 22957b565..ce150f808 100644
--- a/spec/services/retry_service_spec.rb
+++ b/spec/services/retry_service_spec.rb
@@ -6,11 +6,11 @@ RSpec.describe RetryService do
       expect( subject ).not_to receive(:sleep)
     end
 
-    it 'returns a tuple :ok and the result' do
-      expect( subject.execute { 42 } ).to eq([:ok, 42])
+    it 'returns an ok result' do
+      expect( subject.execute { 42 } ).to eq(Result.ok(42))
     end
     it 'does not fail on nil' do
-      expect( subject.execute { nil } ).to eq([:ok, nil])
+      expect( subject.execute { nil } ).to eq(Result.ok(nil))
     end
 
     it 'fails wihout retries if raising un unregistered exception' do
@@ -26,13 +26,13 @@ RSpec.describe RetryService do
     end
     it 'fails after raising a registered exception n times' do
       result = subject.execute{ raise ArgumentError }
-      expect( result.first ).to eq(:error)
-      expect( result.last ).to be_kind_of(ArgumentError)
+      expect( result.status ).to eq(:error)
+      expect( result.value ).to be_kind_of(ArgumentError)
     end
     it 'fails with an explicit try again (automatically registered exception)'  do
       result = subject.execute{ raise RetryService::Retry }
-      expect( result.first ).to eq(:error)
-      expect( result.last ).to be_kind_of(RetryService::Retry)
+      expect( result.status ).to eq(:error)
+      expect( result.value ).to be_kind_of(RetryService::Retry)
     end
   end
 
@@ -43,11 +43,11 @@ RSpec.describe RetryService do
     end
 
     it 'succeds the second time' do
-      expect( subject.execute{ succeed_later(ArgumentError){ 42 } } ).to eq([:ok, 42])
+      expect( subject.execute{ succeed_later(ArgumentError){ 42 } } ).to eq(Result.ok(42))
     end
 
     it 'succeeds the second time with try again (automatically registered exception)' do
-      expect( subject.execute{ succeed_later(RetryService::Retry){ 42 } } ).to eq([:ok, 42])
+      expect( subject.execute{ succeed_later(RetryService::Retry){ 42 } } ).to eq(Result.ok(42))
     end
   end
 
@@ -58,17 +58,25 @@ RSpec.describe RetryService do
       expect( subject ).to receive(:sleep).with(3)
     end
     it 'succeeds the third time with try again (automatically registered exception)' do
-      expect( subject.execute{ succeed_later(RetryService::Retry, count: 2){ 42 } } ).to eq([:ok, 42])
+      expect( subject.execute{ succeed_later(RetryService::Retry, count: 2){ 42 } } ).to eq(Result.ok(42))
     end
   end
 
   context 'failure callback once' do 
+    subject do 
+      described_class.new delays: [2, 3], rescue_from: [NameError, ArgumentError] do |reason, count|
+        @reason=reason
+        @callback_count=count
+        @failures += 1 
+      end
+    end
+
     before do
       @failures = 0
       @count    = 0
       expect( subject ).to receive(:sleep).with(2)
-      subject.register_failure_callback { |reason, count| @reason=reason; @callback_count=count; @failures += 1 }
     end
+
     it 'succeeds the second time and calls the failure_callback once' do
       subject.execute{ succeed_later(RetryService::Retry){ 42 } }
       expect( @failures ).to eq(1)
@@ -81,13 +89,19 @@ RSpec.describe RetryService do
   end
 
   context 'failure callback twice' do
+    subject do 
+      described_class.new delays: [2, 3], rescue_from: [NameError, ArgumentError] do |_reason, _count|
+        @failures += 1 
+      end
+    end
+
     before do
       @failures = 0
       @count    = 0
       expect( subject ).to receive(:sleep).with(2)
       expect( subject ).to receive(:sleep).with(3)
-      subject.register_failure_callback { @failures += 1 }
     end
+
     it 'succeeds the third time and calls the failure_callback twice' do
       subject.execute{ succeed_later(NameError, count: 2){ 42 } }
       expect( @failures ).to eq(2)
diff --git a/spec/workers/workbench_import_worker_spec.rb b/spec/workers/workbench_import_worker_spec.rb
index b6057b36a..03206113c 100644
--- a/spec/workers/workbench_import_worker_spec.rb
+++ b/spec/workers/workbench_import_worker_spec.rb
@@ -6,13 +6,18 @@ RSpec.describe WorkbenchImportWorker, type: [:worker, :request] do
   let( :workbench ){ import.workbench }
   let( :referential ){ import.referential }
   let( :api_key ){ build_stubbed :api_key, referential: referential, token: "#{referential.id}-#{SecureRandom.hex}" }
-  let( :params ){ {referential_id: referential.id, workbench_id: workbench.id} }
+  let( :params ) do
+    { netex_import:
+      { referential_id: referential.id, workbench_id: workbench.id }
+    }
+  end
 
   # http://www.example.com/workbenches/:workbench_id/imports/:id/download
   let( :host ){ Rails.configuration.front_end_host }
   let( :path ){ download_workbench_import_path(workbench, import) }
 
   let( :downloaded_zip ){ double("downloaded zip") }
+  let( :download_zip_response ){ OpenStruct.new( body: downloaded_zip ) }
   let( :download_token ){ SecureRandom.urlsafe_base64 }
 
 
@@ -54,7 +59,7 @@ RSpec.describe WorkbenchImportWorker, type: [:worker, :request] do
 
       expect(HTTPService).to receive(:get_resource)
         .with(host: host, path: path, params: {token: download_token})
-        .and_return( downloaded_zip )
+        .and_return( download_zip_response )
 
       entry_groups.each do | entry_group_name, entry_group_stream |
         mock_post entry_group_name, entry_group_stream, post_response_ok
@@ -76,7 +81,7 @@ RSpec.describe WorkbenchImportWorker, type: [:worker, :request] do
     it 'downloads a zip file, cuts it, and uploads some pieces' do
       expect(HTTPService).to receive(:get_resource)
         .with(host: host, path: path, params: {token: download_token})
-        .and_return( downloaded_zip )
+        .and_return( download_zip_response )
 
       # First entry_group succeeds
       entry_groups[0..0].each do | entry_group_name, entry_group_stream |
@@ -106,7 +111,6 @@ RSpec.describe WorkbenchImportWorker, type: [:worker, :request] do
     expect( HTTPService ).to receive(:post_resource)
       .with(host: host,
             path: upload_path,
-            resource_name: 'netex_import',
             token: api_key.token,
             params: params,
             upload: {file: [entry_group_stream, 'application/zip', entry_group_name]})