Merge remote-tracking branch 'upstream/pull/2120'

Gemfile

@@ -114,6 +114,9 @@ gem "canonical-rails"
 # Used to generate logstash friendly log files
 gem "logstasher"
 
+# Used to generate images for traces
+gem "gd2-ffij"
+
 # Gems useful for development
 group :development do
   gem "annotate"
@@ -125,6 +128,7 @@ end
 
 # Gems needed for running tests
 group :test do
+  gem "fakefs", :require => "fakefs/safe"
   gem "minitest", "~> 5.1", :platforms => [:ruby_19, :ruby_20]
   gem "rails-controller-testing"
   gem "rubocop"

Gemfile.lock

@@ -117,10 +117,13 @@ GEM
     factory_bot_rails (4.11.1)
       factory_bot (~> 4.11.1)
       railties (>= 3.0.0)
+    fakefs (0.18.1)
     faraday (0.15.4)
       multipart-post (>= 1.2, < 3)
     ffi (1.9.25)
     fspath (3.1.0)
+    gd2-ffij (0.3.0)
+      ffi (>= 1.0.0)
     geoip (1.6.4)
     globalid (0.4.1)
       activesupport (>= 4.2.0)
@@ -401,7 +404,9 @@ DEPENDENCIES
   delayed_job_active_record
   dynamic_form
   factory_bot_rails
+  fakefs
   faraday
+  gd2-ffij
   geoip
   htmlentities
   http_accept_language (~> 2.0.0)

app/controllers/traces_controller.rb

@@ -127,6 +127,7 @@ class TracesController < ApplicationController
         flash[:notice] = t ".trace_uploaded"
         flash[:warning] = t ".traces_waiting", :count => current_user.traces.where(:inserted => false).count if current_user.traces.where(:inserted => false).count > 4
 
+        TraceImporterJob.perform_later(@trace) if TRACE_USE_JOB_QUEUE
         redirect_to :action => :index, :display_name => current_user.display_name
       else
         flash[:error] = t("traces.create.upload_failed") if @trace.valid?
@@ -210,6 +211,7 @@ class TracesController < ApplicationController
       trace.visible = false
       trace.save
       flash[:notice] = t ".scheduled_for_deletion"
+      TraceDestroyerJob.perform_later(trace) if TRACE_USE_JOB_QUEUE
       redirect_to :action => :index, :display_name => trace.user.display_name
     end
   rescue ActiveRecord::RecordNotFound

app/jobs/application_job.rb

@@ -0,0 +1,7 @@
+class ApplicationJob < ActiveJob::Base
+  # Automatically retry jobs that encountered a deadlock
+  # retry_on ActiveRecord::Deadlocked
+
+  # Most jobs are safe to ignore if the underlying records are no longer available
+  # discard_on ActiveJob::DeserializationError
+end

app/jobs/trace_destroyer_job.rb

@@ -0,0 +1,10 @@
+class TraceDestroyerJob < ApplicationJob
+  queue_as :default
+
+  def perform(trace)
+    trace.destroy
+  rescue StandardError => ex
+    logger.info ex.to_s
+    ex.backtrace.each { |l| logger.info l }
+  end
+end

app/jobs/trace_importer_job.rb

@@ -0,0 +1,19 @@
+class TraceImporterJob < ApplicationJob
+  queue_as :default
+
+  def perform(trace)
+    gpx = trace.import
+
+    if gpx.actual_points.positive?
+      Notifier.gpx_success(trace, gpx.actual_points).deliver_later
+    else
+      Notifier.gpx_failure(trace, "0 points parsed ok. Do they all have lat,lng,alt,timestamp?").deliver_later
+      trace.destroy
+    end
+  rescue StandardError => ex
+    logger.info ex.to_s
+    ex.backtrace.each { |l| logger.info l }
+    Notifier.gpx_failure(trace, ex.to_s + "\n" + ex.backtrace.join("\n")).deliver_later
+    trace.destroy
+  end
+end

config/example.application.yml

@@ -60,6 +60,10 @@ defaults: &defaults
   spam_threshold: 50
   # Default legale (jurisdiction location) for contributor terms
   default_legale: GB
+  # Use the built-in jobs queue for importing traces
+  # Leave as false if you are using the external high-speed gpx importer
+  # https://github.com/openstreetmap/gpx-import
+  trace_use_job_queue: false
   # Location of GPX traces and images
   gpx_trace_dir: "/home/osm/traces"
   gpx_image_dir: "/home/osm/images"

lib/daemons/gpx_import.rb

@@ -1,59 +0,0 @@
-#!/usr/bin/env ruby
-
-# You might want to change this
-# ENV["RAILS_ENV"] ||= "development"
-
-require File.dirname(__FILE__) + "/../../config/environment"
-
-terminated = false
-
-logger = ActiveRecord::Base.logger
-
-loop do
-  ActiveRecord::Base.logger.info("GPX Import daemon wake @ #{Time.now}.")
-
-  Trace.find(:all, :conditions => { :inserted => false, :visible => true }, :order => "id").each do |trace|
-    Signal.trap("TERM") do
-      terminated = true
-    end
-
-    begin
-      gpx = trace.import
-
-      if gpx.actual_points.positive?
-        Notifier.gpx_success(trace, gpx.actual_points).deliver
-      else
-        Notifier.gpx_failure(trace, "0 points parsed ok. Do they all have lat,lng,alt,timestamp?").deliver
-        trace.destroy
-      end
-    rescue StandardError => ex
-      logger.info ex.to_s
-      ex.backtrace.each { |l| logger.info l }
-      Notifier.gpx_failure(trace, ex.to_s + "\n" + ex.backtrace.join("\n")).deliver
-      trace.destroy
-    end
-
-    Signal.trap("TERM", "DEFAULT")
-
-    exit if terminated
-  end
-
-  Trace.find(:all, :conditions => { :visible => false }, :order => "id").each do |trace|
-    Signal.trap("TERM") do
-      terminated = true
-    end
-
-    begin
-      trace.destroy
-    rescue StandardError => ex
-      logger.info ex.to_s
-      ex.backtrace.each { |l| logger.info l }
-    end
-
-    Signal.trap("TERM", "DEFAULT")
-
-    exit if terminated
-  end
-
-  sleep 5.minutes.value
-end

lib/daemons/gpx_import_ctl

@@ -1,23 +0,0 @@
-#!/usr/bin/env ruby
-require "rubygems"
-require "daemons"
-require "yaml"
-require "erb"
-
-class Hash
-  def with_symbols!
-    each_key { |key| self[key.to_s.to_sym] = self[key] }
-    self
-  end
-end
-
-options = YAML.safe_load(
-  ERB.new(
-    IO.read(
-      File.dirname(__FILE__) + "/../../config/daemons.yml"
-    )
-  ).result
-).with_symbols!
-options[:dir_mode] = options[:dir_mode].to_sym
-
-Daemons.run File.dirname(__FILE__) + "/gpx_import.rb", options

lib/gpx.rb

@@ -45,71 +45,47 @@ module GPX
       end
     end
 
-    def picture(min_lat, min_lon, max_lat, max_lon, num_points)
-      frames = 10
+    def picture(min_lat, min_lon, max_lat, max_lon, _num_points)
+      # frames = 10
       width = 250
       height = 250
       proj = OSM::Mercator.new(min_lat, min_lon, max_lat, max_lon, width, height)
 
-      linegc = Magick::Draw.new
-      linegc.stroke_linejoin("miter")
-      linegc.stroke_width(1)
-      linegc.stroke("#BBBBBB")
-      linegc.fill("#BBBBBB")
+      # TODO: create animated gif
+      # https://github.com/openstreetmap/openstreetmap-website/issues/281
+      image = GD2::Image::IndexedColor.new(width, height)
 
-      highlightgc = Magick::Draw.new
-      highlightgc.stroke_linejoin("miter")
-      highlightgc.stroke_width(3)
-      highlightgc.stroke("#000000")
-      highlightgc.fill("#000000")
+      black = image.palette.allocate(GD2::Color[0, 0, 0])
+      white = image.palette.allocate(GD2::Color[255, 255, 255])
 
-      images = Array(frames) do
-        Magick::Image.new(width, height) do |image|
-          image.background_color = "white"
-          image.format = "GIF"
+      image.draw do |pen|
+        pen.color = white
+        pen.rectangle(0, 0, width, height, true)
+      end
+
+      image.draw do |pen|
+        pen.color = black
+        pen.anti_aliasing = true
+        pen.dont_blend = false
+
+        oldpx = 0.0
+        oldpy = 0.0
+
+        first = true
+
+        points do |p|
+          px = proj.x(p.longitude)
+          py = proj.y(p.latitude)
+
+          pen.line(px, py, oldpx, oldpy) unless first
+
+          first = false
+          oldpy = py
+          oldpx = px
         end
       end
 
-      oldpx = 0.0
-      oldpy = 0.0
-
-      m = 0
-      mm = 0
-      points do |p|
-        px = proj.x(p.longitude)
-        py = proj.y(p.latitude)
-
-        if m.positive?
-          frames.times do |n|
-            gc = if n == mm
-                   highlightgc.dup
-                 else
-                   linegc.dup
-                 end
-
-            gc.line(px, py, oldpx, oldpy)
-
-            gc.draw(images[n])
-          end
-        end
-
-        m += 1
-        mm += 1 if m > num_points.to_f / frames.to_f * (mm + 1)
-
-        oldpy = py
-        oldpx = px
-      end
-
-      il = Magick::ImageList.new
-
-      images.each do |f|
-        il << f
-      end
-
-      il.delay = 50
-      il.format = "GIF"
-
-      il.to_blob
+      image.gif
     end
 
     def icon(min_lat, min_lon, max_lat, max_lon)
@@ -117,34 +93,39 @@ module GPX
       height = 50
       proj = OSM::Mercator.new(min_lat, min_lon, max_lat, max_lon, width, height)
 
-      gc = Magick::Draw.new
-      gc.stroke_linejoin("miter")
-      gc.stroke_width(1)
-      gc.stroke("#000000")
-      gc.fill("#000000")
+      image = GD2::Image::IndexedColor.new(width, height)
 
-      image = Magick::Image.new(width, height) do |i|
-        i.background_color = "white"
-        i.format = "GIF"
+      black = image.palette.allocate(GD2::Color[0, 0, 0])
+      white = image.palette.allocate(GD2::Color[255, 255, 255])
+
+      image.draw do |pen|
+        pen.color = white
+        pen.rectangle(0, 0, width, height, true)
       end
 
-      oldpx = 0.0
-      oldpy = 0.0
+      image.draw do |pen|
+        pen.color = black
+        pen.anti_aliasing = true
+        pen.dont_blend = false
 
-      first = true
+        oldpx = 0.0
+        oldpy = 0.0
 
-      points do |p|
-        px = proj.x(p.longitude)
-        py = proj.y(p.latitude)
+        first = true
 
-        gc.dup.line(px, py, oldpx, oldpy).draw(image) unless first
+        points do |p|
+          px = proj.x(p.longitude)
+          py = proj.y(p.latitude)
 
-        first = false
-        oldpy = py
-        oldpx = px
+          pen.line(px, py, oldpx, oldpy) unless first
+
+          first = false
+          oldpy = py
+          oldpx = px
+        end
       end
 
-      image.to_blob
+      image.gif
     end
   end
 

test/jobs/trace_destroyer_job_test.rb

@@ -0,0 +1,18 @@
+require "test_helper"
+require "minitest/mock"
+
+class TraceDestroyerJobTest < ActiveJob::TestCase
+  def test_destroy_called
+    trace = Minitest::Mock.new
+
+    # Tiny little bit of mocking to make activejob happy
+    trace.expect :is_a?, false, [TraceDestroyerJob]
+
+    # Check that trace.destroy is called
+    trace.expect :destroy, true
+
+    TraceDestroyerJob.perform_now(trace)
+
+    assert_mock trace
+  end
+end

test/jobs/trace_importer_job_test.rb

@@ -0,0 +1,64 @@
+require "test_helper"
+require "minitest/mock"
+
+class TraceImporterJobTest < ActiveJob::TestCase
+  def test_success_notification
+    # Check that the user gets a success notification when the trace has valid points
+    trace = create(:trace)
+
+    gpx = Minitest::Mock.new
+    def gpx.actual_points
+      5
+    end
+
+    trace.stub(:import, gpx) do
+      perform_enqueued_jobs do
+        TraceImporterJob.perform_now(trace)
+      end
+    end
+
+    assert_performed_jobs 1
+
+    email = ActionMailer::Base.deliveries.last
+    assert_equal trace.user.email, email.to[0]
+    assert_match(/success/, email.subject)
+  end
+
+  def test_failure_notification
+    # Check that the user gets a failure notification when the trace has no valid points
+    trace = create(:trace)
+
+    gpx = Minitest::Mock.new
+    def gpx.actual_points
+      0
+    end
+
+    trace.stub(:import, gpx) do
+      perform_enqueued_jobs do
+        TraceImporterJob.perform_now(trace)
+      end
+    end
+
+    assert_performed_jobs 1
+
+    email = ActionMailer::Base.deliveries.last
+    assert_equal trace.user.email, email.to[0]
+    assert_match(/failure/, email.subject)
+  end
+
+  def test_error_notification
+    # Check that the user gets a failure notification when something goes badly wrong
+    trace = create(:trace)
+    trace.stub(:import, -> { raise }) do
+      perform_enqueued_jobs do
+        TraceImporterJob.perform_now(trace)
+      end
+    end
+
+    assert_performed_jobs 1
+
+    email = ActionMailer::Base.deliveries.last
+    assert_equal trace.user.email, email.to[0]
+    assert_match(/failure/, email.subject)
+  end
+end

test/models/trace_test.rb

@@ -192,6 +192,66 @@ class TraceTest < ActiveSupport::TestCase
     trace.destroy
   end
 
+  # When testing the trace.import method, care needs to be taken regarding the icon
+  # fixture files, since the fixtures could be overwritten by newly generated files.
+  # We use FakeFS to temporarily protect the real fixture files from being overwritten.
+
+  def test_import_removes_previous_tracepoints
+    FakeFS do
+      FakeFS::FileSystem.clone(Rails.root.join("test", "gpx"))
+      trace = create(:trace, :fixture => "a")
+      # Tracepoints don't have a primary key, so we use a specific latitude to
+      # check for successful deletion
+      create(:tracepoint, :latitude => 54321, :trace => trace)
+      assert_equal 1, Tracepoint.where(:latitude => 54321).count
+
+      trace.import
+
+      assert_equal 0, Tracepoint.where(:latitude => 54321).count
+    end
+  end
+
+  def test_import_creates_tracepoints
+    FakeFS do
+      FakeFS::FileSystem.clone(Rails.root.join("test", "gpx"))
+      trace = create(:trace, :fixture => "a")
+      assert_equal 0, Tracepoint.where(:gpx_id => trace.id).count
+
+      trace.import
+
+      trace.reload
+      assert_equal 1, Tracepoint.where(:gpx_id => trace.id).count
+    end
+  end
+
+  def test_import_creates_icon
+    FakeFS do
+      FakeFS::FileSystem.clone(Rails.root.join("test", "gpx"))
+      trace = create(:trace, :fixture => "a")
+      icon_path = File.join(GPX_IMAGE_DIR, "#{trace.id}_icon.gif")
+      FileUtils.rm(icon_path)
+      assert_equal false, File.exist?(icon_path)
+
+      trace.import
+
+      assert_equal true, File.exist?(icon_path)
+    end
+  end
+
+  def test_import_creates_large_picture
+    FakeFS do
+      FakeFS::FileSystem.clone(Rails.root.join("test", "gpx"))
+      trace = create(:trace, :fixture => "a")
+      large_picture_path = File.join(GPX_IMAGE_DIR, "#{trace.id}.gif")
+      FileUtils.rm(large_picture_path)
+      assert_equal false, File.exist?(large_picture_path)
+
+      trace.import
+
+      assert_equal true, File.exist?(large_picture_path)
+    end
+  end
+
   private
 
   def check_query(query, traces)