Merge pull request #363 from roomorama/feature/improved-skip-stats

feature/improved-skip-stats

CHANGELOG.md

@@ -7,6 +7,8 @@ how this file is formatted and how the process works.
 ## Unreleased
 ### Added
 - Atleisure:: calendar sync worker
+- reason for all properties skipped during sync
+- page for sync process stats
 
 ### Fixed
 - Ciirus:: skip properties without images

apps/web/config/routes.rb

@@ -1,4 +1,5 @@
-get "/", to: "dashboard#index",       as: :root
+get "/sync_processes/:id/stats", to: "sync_processes#stats", as: :stats
+get "/",                         to: "dashboard#index",      as: :root
 
 resources :errors,         only: [:index, :show], controller: "external_errors"
 resources :reservations,   only: [:index]

apps/web/controllers/external_errors/show.rb

@@ -10,16 +10,7 @@ class Show
     def call(params)
       @error = ExternalErrorRepository.find(params[:id])
 
-      unless @error
-        render_404_template
-      end
-    end
-
-    private
-
-    def render_404_template
-      template = Web::Controllers::TemplateRenderer.new("404.html.erb").render
-      status 404, template
+      halt 404 unless @error
     end
   end
 end

apps/web/controllers/sync_processes/stats.rb

@@ -0,0 +1,14 @@
+module Web::Controllers::SyncProcesses
+  class Stats
+    include Web::Action
+    include Web::Controllers::InternalError
+
+    expose :sync_process
+
+    def call(params)
+      @sync_process = SyncProcessRepository.find(params[:id])
+
+      halt 404 unless @sync_process
+    end
+  end
+end

apps/web/templates/sync_processes/index.html.erb

@@ -21,6 +21,7 @@
         <th>Properties Deleted</th>
         <th>Properties Updated</th>
         <th>Properties Skipped</th>
+        <th>Stats</th>
       </tr>
     </thead>
 
@@ -36,7 +37,8 @@
           <td><%= format_number sync.stats[:properties_created] %></td>
           <td><%= format_number sync.stats[:properties_updated] %></td>
           <td><%= format_number sync.stats[:properties_deleted] %></td>
-          <td><%= format_number sync.stats[:properties_skipped] %></td>
+          <td><%= format_number sync.skipped_properties_count %></td>
+          <td><%= link_to(image("stats16.png"), routes.stats_path(sync.id))%></td>
         <% end %>
       </tr>
     </tbody>

apps/web/templates/sync_processes/stats.html.erb

@@ -0,0 +1,6 @@
+<div class="content">
+  <h2>Sync Process Stats</h2>
+  <pre class="highlight json code-block">
+<%= pretty_print_json(sync_process.stats) %>
+  </pre>
+</div>

apps/web/views/sync_processes/stats.rb

@@ -0,0 +1,26 @@
+module Web::Views::SyncProcesses
+  class Stats
+    include Web::View
+    include Concierge::JSON
+
+    def pretty_print_json(content)
+
+      # uses the +pretty+ and +indent+ options provided by +Yajl::Encoder+ to
+      # format the parsed JSON. Generates two line breaks per line (not for empty arrays)
+      # to make the final content more readable.
+      compact_empty_arrays(
+        double_line_breaks Yajl::Encoder.encode(content.to_h, pretty: true, indent: " " * 2)
+      )
+    end
+
+    private
+
+    def double_line_breaks(str)
+      str.gsub("\n", "\n\n")
+    end
+
+    def compact_empty_arrays(str)
+      str.gsub(/\[\s*\]/, '[]')
+    end
+  end
+end

apps/workers/property_synchronisation.rb

@@ -35,18 +35,19 @@ class PropertySynchronisation
     # Calendar availabilities is tackled by +Workers::CalendarSynchronisation+
     WORKER_TYPE = "metadata"
 
-    PropertyCounters = Struct.new(:created, :updated, :deleted, :skipped)
+    PropertyCounters = Struct.new(:created, :updated, :deleted)
 
-    attr_reader :host, :router, :sync_record, :counters, :processed, :purge
+    attr_reader :host, :router, :sync_record, :counters, :processed, :purge, :properties_skipped
 
     # host - an instance of +Host+.
     def initialize(host)
-      @host        = host
-      @router      = Workers::Router.new(host)
-      @sync_record = init_sync_record(host)
-      @counters    = PropertyCounters.new(0, 0, 0, 0)
-      @processed   = []
-      @purge       = true
+      @host               = host
+      @router             = Workers::Router.new(host)
+      @sync_record        = init_sync_record(host)
+      @counters           = PropertyCounters.new(0, 0, 0)
+      @processed          = []
+      @purge              = true
+      @properties_skipped = Hash.new { |hsh, key| hsh[key] = [] }
     end
 
     # Indicates that the property with the given +identifier+ is being synchronised.
@@ -136,8 +137,8 @@ def finish!
     #   synchronisation.skip_property
     # end
     #
-    def skip_property
-      counters.skipped += 1
+    def skip_property(property_id, reason)
+      properties_skipped[reason] << property_id
     end
 
     # Used to initialize a clean context for a property id.
@@ -210,12 +211,21 @@ def save_sync_process
       sync_record.stats[:properties_created] = counters.created
       sync_record.stats[:properties_updated] = counters.updated
       sync_record.stats[:properties_deleted] = counters.deleted
-      sync_record.stats[:properties_skipped] = counters.skipped
+      sync_record.stats[:properties_skipped] = prepare_properties_skipped
       sync_record.finished_at = Time.now
 
       database.create(sync_record)
     end
 
+    def prepare_properties_skipped
+      properties_skipped.map do |msg, ids|
+        {
+          'reason' => msg,
+          'ids' => ids
+        }
+      end
+    end
+
     def run_operation(operation, *args)
       # enable context tracking when performing API calls to Roomorama so that
       # any errors during the request can be logged.

apps/workers/suppliers/atleisure/metadata.rb

@@ -39,8 +39,9 @@ def fetch_data_and_process(properties)
       if result.success?
         properties_data = result.value
         properties_data.map do |property|
+          property_id = property['HouseCode']
           if validator(property).valid?
-            synchronisation.start(property['HouseCode']) {
+            synchronisation.start(property_id) {
               # AtLeisure's API calls return with large result payloads while
               # synchronising properties, therefore, event tracking is disabled
               # while the property is parsed.
@@ -49,7 +50,7 @@ def fetch_data_and_process(properties)
               mapper.prepare(property)
             }
           else
-            synchronisation.skip_property
+            synchronisation.skip_property(property_id, 'Invalid property')
           end
         end
       else

apps/workers/suppliers/ciirus/metadata.rb

@@ -36,7 +36,7 @@ def perform
             next unless permissions.success?
 
             unless permissions_validator(permissions.value).valid?
-              synchronisation.skip_property
+              synchronisation.skip_property(property_id, 'Invalid permissions')
               next
             end
 
@@ -47,7 +47,7 @@ def perform
 
             rates  = filter_rates(result.value)
             if rates.empty?
-              synchronisation.skip_property
+              synchronisation.skip_property(property_id, 'Empty valid rates list')
               next
             end
 
@@ -59,7 +59,7 @@ def perform
             next unless result.success?
             description = result.value
             if description.to_s.empty?
-              synchronisation.skip_property
+              synchronisation.skip_property(property_id, 'Empty description')
               next
             end
 
@@ -72,7 +72,7 @@ def perform
               Result.new(roomorama_property)
             end
           else
-            synchronisation.skip_property
+            synchronisation.skip_property(property_id, 'Invalid property')
           end
         end
         synchronisation.finish!
@@ -104,7 +104,7 @@ def fetch_images(property_id)
       importer.fetch_images(property_id).tap do |result|
         unless result.success?
           if ignorable_images_error?(result.error)
-            synchronisation.skip_property
+            synchronisation.skip_property(property_id, "Ignorable images error: #{result.error.data}")
           else
             message = "Failed to fetch images for property `#{property_id}`"
             announce_error(message, result)
@@ -124,7 +124,7 @@ def fetch_rates(property_id)
       importer.fetch_rates(property_id).tap do |result|
         unless result.success?
           if ignorable_rates_error?(result.error)
-            synchronisation.skip_property
+            synchronisation.skip_property(property_id, "Ignorable rates error: #{result.error.data}")
           else
             message = "Failed to fetch rates for property `#{property_id}`"
             announce_error(message, result)

apps/workers/suppliers/poplidays/metadata.rb

@@ -16,14 +16,13 @@ def perform
       if result.success?
         properties = result.value
         properties.each do |property|
+          property_id = property['id']
           if validator(property).valid?
-            property_id = property['id']
-
             details = synchronisation.new_context { fetch_property_details(property_id) }
             next unless details.success?
 
             unless details_validator(details.value).valid?
-              synchronisation.skip_property
+              synchronisation.skip_property(property_id, 'Invalid property details')
               next
             end
 
@@ -33,7 +32,7 @@ def perform
             availabilities = filter_availabilities(result.value)
 
             if availabilities.empty?
-              synchronisation.skip_property
+              synchronisation.skip_property(property_id, 'Empty valid availabilities list')
               next
             end
 
@@ -44,7 +43,7 @@ def perform
               mapper.build(property, details.value, availabilities, extras)
             end
           else
-            synchronisation.skip_property
+            synchronisation.skip_property(property_id, 'Invalid property')
           end
         end
         synchronisation.finish!

db/migrations/20160919094714_change_properties_skipped_stat.rb

@@ -0,0 +1,26 @@
+Hanami::Model.migration do
+  up do
+    SyncProcessRepository.of_type('metadata').each do |sync_record|
+      stats = sync_record.stats.to_h
+      stats['properties_skipped'] = [
+        {
+          'reason' => 'Unknown reason',
+          'ids' => Array.new(stats['properties_skipped'], '')
+        }
+      ]
+      sync_record.stats = stats
+      SyncProcessRepository.update(sync_record)
+    end
+  end
+
+  down do
+    SyncProcessRepository.of_type('metadata').each do |sync_record|
+      stats = sync_record.stats.to_h
+      stats['properties_skipped'] = stats['properties_skipped'].inject(0) do |res, ps|
+        res + ps['ids'].length
+      end
+      sync_record.stats = stats
+      SyncProcessRepository.update(sync_record)
+    end
+  end
+end

lib/concierge/entities/sync_process.rb

@@ -27,4 +27,10 @@ class SyncProcess
 
   attributes :id, :host_id, :started_at, :finished_at, :successful, :type,
     :stats, :created_at, :updated_at
+
+  # Method actual only for metadata sync process.
+  # Always returns 0 for others types.
+  def skipped_properties_count
+    stats[:properties_skipped].map { |h| Array(h['ids']).size }.reduce(0, :+)
+  end
 end

spec/web/views/sync_processes/index_spec.rb

@@ -22,7 +22,13 @@
     create_sync_process(type: "metadata", successful: false, host_id: metadata_host.id, stats: {
       properties_created: 2,
       properties_updated: 10,
-      properties_deleted: 1
+      properties_deleted: 1,
+      properties_skipped: [
+        {
+          reason: 'Error 1',
+          ids: ['314', '345']
+        }
+      ]
     })
 
     create_sync_process(type: "availabilities", successful: true, host_id: availabilities_host.id, stats: {
@@ -39,6 +45,7 @@
     expect(sanitized).to include %(<td>2</td>)  # properties created
     expect(sanitized).to include %(<td>10</td>) # properties updated
     expect(sanitized).to include %(<td>1</td>)  # properties deleted
+    expect(sanitized).to include %(<td>2</td>)  # properties skipped
   end
 
   it "includes information about availabilities sync processes" do

spec/web/views/sync_processes/stats_spec.rb

@@ -0,0 +1,39 @@
+require "spec_helper"
+require_relative '../../../../apps/web/views/sync_processes/stats'
+
+RSpec.describe Web::Views::SyncProcesses::Stats do
+  include Support::Factories
+
+  let(:metadata)  { SyncProcessRepository.all.first }
+  let(:exposures) { Hash[sync_process: metadata] }
+  let(:template)  { Hanami::View::Template.new('apps/web/templates/sync_processes/stats.html.erb') }
+  let(:view)      { described_class.new(template, exposures) }
+  let(:rendered)  { view.render }
+
+
+  let(:sanitized) { rendered.gsub("\n", "").gsub(/\s\s+/, " ") }
+
+  before do
+    metadata_supplier = create_supplier(name: "Metadata Supplier")
+    metadata_host     = create_host(supplier_id: metadata_supplier.id, username: "metadata-host")
+
+    create_sync_process(type: "metadata", successful: false, host_id: metadata_host.id, stats: {
+      properties_created: 2,
+      properties_updated: 10,
+      properties_deleted: 1,
+      properties_skipped: [
+        {
+          reason: 'Error 1',
+          ids: ['314', '345']
+        }
+      ]
+    })
+  end
+
+  it "includes information about metadata sync processes" do
+    expect(sanitized).to include %(: 2)  # properties created
+    expect(sanitized).to include %(: 10) # properties updated
+    expect(sanitized).to include %(: 1)  # properties deleted
+    expect(sanitized).to include %(314)  # skipped property
+  end
+end