From 6138e37a8d21b362c7f420afb49d503e1b1eb040 Mon Sep 17 00:00:00 2001 From: Pablo Brasero Date: Thu, 19 Mar 2026 15:23:43 +0000 Subject: [PATCH] Use Noticed to deliver notifications on GPX import failure The previous code relied on `#deliver` (alias for `#deliver_now`) to immediately deliver the email so that the trace could be deleted. To work around this, we pass the details of the trace instead of the trace record, which won't be available by the time the notifications are ultimately delivered. This means the trace is not there to be the `record`, so we are not passing one. Since we can't get the recipient from the non-existing record, we give the recipient explicitly to `#deliver_later`. Additionally, the mailer method now needs to receive the recipient directly, instead of extracting it from the trace record. --- app/jobs/trace_importer_job.rb | 19 +++++++++---- app/mailers/user_mailer.rb | 24 +++++++++++----- app/notifiers/gpx_import_failure_notifier.rb | 8 ++++++ test/jobs/trace_importer_job_test.rb | 16 ++++++++--- test/mailers/previews/user_mailer_preview.rb | 8 +++++- test/mailers/user_mailer_test.rb | 30 +++++++++++++++----- 6 files changed, 81 insertions(+), 24 deletions(-) create mode 100644 app/notifiers/gpx_import_failure_notifier.rb diff --git a/app/jobs/trace_importer_job.rb b/app/jobs/trace_importer_job.rb index 108d300c9..231105fd0 100644 --- a/app/jobs/trace_importer_job.rb +++ b/app/jobs/trace_importer_job.rb @@ -9,17 +9,26 @@ class TraceImporterJob < ApplicationJob if gpx.actual_points.positive? GpxImportSuccessNotifier.with(:record => trace, :possible_points => gpx.actual_points).deliver_later else - UserMailer.with(:trace => trace, :error => "0 points parsed ok. Do they all have lat,lng,alt,timestamp?").gpx_failure.deliver - trace.destroy + handle_import_failure_notification(trace, "0 points parsed ok. Do they all have lat,lng,alt,timestamp?") end rescue LibXML::XML::Error => e logger.info e.to_s - UserMailer.with(:trace => trace, :error => e).gpx_failure.deliver - trace.destroy + handle_import_failure_notification(trace, e.to_s) rescue StandardError => e logger.info e.to_s e.backtrace.each { |l| logger.info l } - UserMailer.with(:trace => trace, :error => "#{e}\n#{e.backtrace.join("\n")}").gpx_failure.deliver + handle_import_failure_notification(trace, "#{e}\n#{e.backtrace.join("\n")}") + end + + private + + def handle_import_failure_notification(trace, error) + GpxImportFailureNotifier.with( + :trace_name => trace.name, + :trace_description => trace.description, + :trace_tags => trace.tags.map(&:tag), + :error => error + ).deliver_later(trace.user) trace.destroy end end diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index 55320c9f8..67749cb84 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -70,16 +70,26 @@ class UserMailer < ApplicationMailer end def gpx_failure - trace, error = params.fetch_values(:trace, :error) + trace_name, + trace_description, + trace_tags, + error, + recipient = params.fetch_values( + :trace_name, + :trace_description, + :trace_tags, + :error, + :recipient + ) - with_recipient_locale trace.user do - @to_user = trace.user.display_name - @trace_name = trace.name - @trace_description = trace.description - @trace_tags = trace.tags + with_recipient_locale recipient do + @to_user = recipient.display_name + @trace_name = trace_name + @trace_description = trace_description + @trace_tags = trace_tags @error = error - mail :to => trace.user.email, + mail :to => recipient.email, :subject => t(".subject") end end diff --git a/app/notifiers/gpx_import_failure_notifier.rb b/app/notifiers/gpx_import_failure_notifier.rb new file mode 100644 index 000000000..598b39a05 --- /dev/null +++ b/app/notifiers/gpx_import_failure_notifier.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class GpxImportFailureNotifier < ApplicationNotifier + deliver_by :email do |config| + config.mailer = "UserMailer" + config.method = "gpx_failure" + end +end diff --git a/test/jobs/trace_importer_job_test.rb b/test/jobs/trace_importer_job_test.rb index 8c5250eed..b96aae75a 100644 --- a/test/jobs/trace_importer_job_test.rb +++ b/test/jobs/trace_importer_job_test.rb @@ -36,7 +36,9 @@ class TraceImporterJobTest < ActiveJob::TestCase end trace.stub(:import, gpx) do - TraceImporterJob.perform_now(trace) + perform_enqueued_jobs do + TraceImporterJob.perform_now(trace) + end end email = ActionMailer::Base.deliveries.last @@ -50,7 +52,9 @@ class TraceImporterJobTest < ActiveJob::TestCase # Check that the user gets a failure notification when something goes badly wrong trace = create(:trace) trace.stub(:import, -> { raise "Test Exception" }) do - TraceImporterJob.perform_now(trace) + perform_enqueued_jobs do + TraceImporterJob.perform_now(trace) + end end email = ActionMailer::Base.deliveries.last @@ -65,7 +69,9 @@ class TraceImporterJobTest < ActiveJob::TestCase def test_parse_error_notification trace = create(:trace, :inserted => false, :fixture => "jpg") Rails.logger.silence do - TraceImporterJob.perform_now(trace) + perform_enqueued_jobs do + TraceImporterJob.perform_now(trace) + end end email = ActionMailer::Base.deliveries.last @@ -80,7 +86,9 @@ class TraceImporterJobTest < ActiveJob::TestCase def test_gz_parse_error_notification trace = create(:trace, :inserted => false, :fixture => "jpg.gz") Rails.logger.silence do - TraceImporterJob.perform_now(trace) + perform_enqueued_jobs do + TraceImporterJob.perform_now(trace) + end end email = ActionMailer::Base.deliveries.last diff --git a/test/mailers/previews/user_mailer_preview.rb b/test/mailers/previews/user_mailer_preview.rb index b418a11ed..eb028a2cf 100644 --- a/test/mailers/previews/user_mailer_preview.rb +++ b/test/mailers/previews/user_mailer_preview.rb @@ -50,7 +50,13 @@ class UserMailerPreview < ActionMailer::Preview rescue LibXML::XML::Error => e e.message end - UserMailer.with(:trace => trace, :error => error).gpx_failure + UserMailer.with( + :trace_name => trace.name, + :trace_description => trace.description, + :trace_tags => trace.tags, + :error => error, + :recipient => trace.user + ).gpx_failure end def message_notification diff --git a/test/mailers/user_mailer_test.rb b/test/mailers/user_mailer_test.rb index 1f3b1af58..c25325183 100644 --- a/test/mailers/user_mailer_test.rb +++ b/test/mailers/user_mailer_test.rb @@ -78,13 +78,29 @@ class UserMailerTest < ActionMailer::TestCase assert_includes email.text_part.body, url end - def test_gpx_failure_no_trace_link - trace = create(:trace) - email = UserMailer.with(:trace => trace, :error => "some error").gpx_failure - url = url_helpers.show_trace_url(trace.user, trace) - - assert_select parse_html_body(email), "a[href='#{url}']", :count => 0 - assert_not_includes email.text_part.body, url + def test_gpx_failure + trace = build(:trace, :tags => build_list(:tracetag, 2)) + email = UserMailer.with( + :trace_name => trace.name, + :trace_description => trace.description, + :trace_tags => trace.tags, + :error => "some error", + :recipient => trace.user + ).gpx_failure + + tags = trace.tags.map(&:tag) + assert_match trace.name, email.html_part.body.to_s + assert_match trace.description, email.html_part.body.to_s + assert_match tags[0], email.html_part.body.to_s + assert_match tags[1], email.html_part.body.to_s + assert_match "some error", email.html_part.body.to_s + + tags = trace.tags.map(&:tag) + assert_match trace.name, email.text_part.body.to_s + assert_match trace.description, email.text_part.body.to_s + assert_match tags[0], email.text_part.body.to_s + assert_match tags[1], email.text_part.body.to_s + assert_match "some error", email.text_part.body.to_s end def test_message_notification -- 2.47.3