From: Tom Hughes Date: Wed, 29 Aug 2018 07:36:22 +0000 (+0100) Subject: Merge remote-tracking branch 'upstream/pull/1961' X-Git-Tag: live~3982 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/db39876dd6f0bccb36e4cdde0e1a0da81a05fcfe?hp=e58d5166dd2a2cc3267d0d9d947e6214bfb4b67d Merge remote-tracking branch 'upstream/pull/1961' --- diff --git a/app/controllers/traces_controller.rb b/app/controllers/traces_controller.rb index 8994d8a2e..ab4c8f3b8 100644 --- 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 a403d2959..5bc88547a 100644 --- 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 456bf9f8a..fbbbe4adc 100644 --- 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")