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

diff --git a/app/controllers/traces_controller.rb b/app/controllers/traces_controller.rb
index 8994d8a2e58df90e17535edf5ae63a6cf08b258e..ab4c8f3b80849408be5259afa1555d20784b8193 100644 (file)
--- a/app/controllers/traces_controller.rb
+++ b/app/controllers/traces_controller.rb
@@ -110,12 +110,14 @@ class TracesController < ApplicationController
   end
 
   def create
+    @title = t ".upload_trace"
+
     logger.info(params[:trace][:gpx_file].class.name)
 
     if params[:trace][:gpx_file].respond_to?(:read)
       begin
-        do_create(params[:trace][:gpx_file], params[:trace][:tagstring],
-                  params[:trace][:description], params[:trace][:visibility])
+        @trace = do_create(params[:trace][:gpx_file], params[:trace][:tagstring],
+                           params[:trace][:description], params[:trace][:visibility])
       rescue StandardError => ex
         logger.debug ex
       end
@@ -125,6 +127,10 @@ class TracesController < ApplicationController
         flash[:warning] = t ".traces_waiting", :count => current_user.traces.where(:inserted => false).count if current_user.traces.where(:inserted => false).count > 4
 
         redirect_to :action => :list, :display_name => current_user.display_name
+      else
+        flash[:error] = t("traces.create.upload_failed") if @trace.valid?
+
+        render :action => "new"
       end
     else
       @trace = Trace.new(:name => "Dummy",
@@ -135,7 +141,7 @@ class TracesController < ApplicationController
                          :timestamp => Time.now.getutc)
       @trace.valid?
       @trace.errors.add(:gpx_file, "can't be blank")
-      @title = t ".upload_trace"
+
       render :action => "new"
     end
   end
@@ -309,11 +315,11 @@ class TracesController < ApplicationController
     end
 
     if params[:file].respond_to?(:read)
-      do_create(params[:file], tags, description, visibility)
+      trace = do_create(params[:file], tags, description, visibility)
 
-      if @trace.id
-        render :plain => @trace.id.to_s
-      elsif @trace.valid?
+      if trace.id
+        render :plain => trace.id.to_s
+      elsif trace.valid?
         head :internal_server_error
       else
         head :bad_request
@@ -337,7 +343,7 @@ class TracesController < ApplicationController
 
     # Create the trace object, falsely marked as already
     # inserted to stop the import daemon trying to load it
-    @trace = Trace.new(
+    trace = Trace.new(
       :name => name,
       :tagstring => tags,
       :description => description,
@@ -347,31 +353,33 @@ class TracesController < ApplicationController
       :timestamp => Time.now.getutc
     )
 
-    Trace.transaction do
-      begin
-        # Save the trace object
-        @trace.save!
-
-        # Rename the temporary file to the final name
-        FileUtils.mv(filename, @trace.trace_name)
-      rescue StandardError
-        # Remove the file as we have failed to update the database
-        FileUtils.rm_f(filename)
-
-        # Pass the exception on
-        raise
-      end
-
-      begin
-        # Clear the inserted flag to make the import daemon load the trace
-        @trace.inserted = false
-        @trace.save!
-      rescue StandardError
-        # Remove the file as we have failed to update the database
-        FileUtils.rm_f(@trace.trace_name)
-
-        # Pass the exception on
-        raise
+    if trace.valid?
+      Trace.transaction do
+        begin
+          # Save the trace object
+          trace.save!
+
+          # Rename the temporary file to the final name
+          FileUtils.mv(filename, trace.trace_name)
+        rescue StandardError
+          # Remove the file as we have failed to update the database
+          FileUtils.rm_f(filename)
+
+          # Pass the exception on
+          raise
+        end
+
+        begin
+          # Clear the inserted flag to make the import daemon load the trace
+          trace.inserted = false
+          trace.save!
+        rescue StandardError
+          # Remove the file as we have failed to update the database
+          FileUtils.rm_f(trace.trace_name)
+
+          # Pass the exception on
+          raise
+        end
       end
     end
 
@@ -382,6 +390,8 @@ class TracesController < ApplicationController
     else
       current_user.preferences.create(:k => "gps.trace.visibility", :v => visibility)
     end
+
+    trace
   end
 
   def offline_warning

diff --git a/config/locales/en.yml b/config/locales/en.yml
index a403d29592d189ecc9f8823c09807c50839ce730..5bc88547a043b3c2df63b7a74129fe712447fda6 100644 (file)
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1700,6 +1700,7 @@ en:
     create:
       upload_trace: "Upload GPS Trace"
       trace_uploaded: "Your GPX file has been uploaded and is awaiting insertion in to the database. This will usually happen within half an hour, and an email will be sent to you on completion."
+      upload_failed: "Sorry, the GPX upload failed. An administrator has been alerted to the error. Please try again"
       traces_waiting:
         one: "You have %{count} trace waiting for upload. Please consider waiting for these to finish before uploading any more, so as not to block the queue for other users."
         other: "You have %{count} traces waiting for upload. Please consider waiting for these to finish before uploading any more, so as not to block the queue for other users."

diff --git a/test/controllers/traces_controller_test.rb b/test/controllers/traces_controller_test.rb
index 456bf9f8a505426cd26d69caee8b39c82fa058ac..fbbbe4adc341bd9d0fe9c932b153253bca080b4b 100644 (file)
--- a/test/controllers/traces_controller_test.rb
+++ b/test/controllers/traces_controller_test.rb
@@ -571,6 +571,21 @@ class TracesControllerTest < ActionController::TestCase
     assert_equal "trackable", user.preferences.where(:k => "gps.trace.visibility").first.v
   end
 
+  # Test creating a trace with validation errors
+  def test_create_post_with_validation_errors
+    # Get file to use
+    fixture = Rails.root.join("test", "gpx", "fixtures", "a.gpx")
+    file = Rack::Test::UploadedFile.new(fixture, "application/gpx+xml")
+    user = create(:user)
+
+    # Now authenticated
+    create(:user_preference, :user => user, :k => "gps.trace.visibility", :v => "identifiable")
+    assert_not_equal "trackable", user.preferences.where(:k => "gps.trace.visibility").first.v
+    post :create, :params => { :trace => { :gpx_file => file, :description => "", :tagstring => "new,trace", :visibility => "trackable" } }, :session => { :user => user }
+    assert_template :new
+    assert_match "Description is too short (minimum is 1 character)", response.body
+  end
+
   # Test fetching the edit page for a trace using GET
   def test_edit_get
     public_trace_file = create(:trace, :visibility => "public")