Use Noticed for notifications on GPX imports

[pablobm]

Following #6837, and sibling to #6933, this converts some more code to use Noticed instead of using mailers directly.

The success notification was the simpler of the two. I only needed to adapt the mailer to use the recipient explicitly given in the params, as opposed to extracting it from the trace. This was raised by @tomhughes during review.

The failure notification is a bit more involved. First the two easy changes:

• Same as with success, the recipient is given explicitly instead of extracted from the trace.
• I changed the test because a) it depended on the trace record being available and b) I don't like that sort of negative assertion it was using.

More importantly, there's the issue that the current code deletes the trace immediately after delivering the email synchronously. To work around this, the notifier will not receive a record, only the metadata as explicit arguments, and the recipient will be given explicitly in the deliver_later call.

In any case, and as mentioned by @gravitystorm, this raises the question of what to do when we implement on-site notifications and these refer to deleted records. This might involve creating a new model OnsiteNotification that doesn't require other records, instead of piggybacking on the existing Noticed::Notifications.

[gravitystorm]

Thoughts?

My initial reaction is that this is the classic ActiveJob model reference problem. Because the job stores a reference to the object, and then looks up the object attributes at some later time, it's not guaranteed that the object still exists when the job runs. This is that same problem on maximum scale, since it affects 100% of the jobs, instead of just a small enough percentage that everyone sweeps the problem under the carpet.

My usual approach for similar circumstances is to serialise the required attributes into the job. This means the job runs without needing to load the object. So something like:

- GpxImportFailureNotifier.with(:record => trace, :error => e.to_s).deliver_later
+ GpxImportFailureNotifier.with(:error => e.to_s, :trace_name => trace.name, :trace_description => trace.description, :trace_tags => ...etc...).deliver_later

However, I don't know if noticed is particularly happy without having a record to associate with. https://github.com/excid3/noticed?tab=readme-ov-file#handling-deleted-records only covers the case where you want to avoid sending notifications for deleted objects, but that's the opposite of what we want here.

This also makes me think about how we would handle this with on-site notifications. What if I upload 10 traces and come back to the website next weekend? How will the system know what to write in the site notifications?

[tomhughes]

Maybe we should just stop deleting the records? We can set visible to false so they're not shown in listings, and delete the attachment, but keep the actual record?

If you really wanted you can have a cleanup job as part of the daily cleanup task I mentioned that removed any that were no longer referenced by a notification, once we have the notification cleanup we discussed before.

[tomhughes]

This is controlling what user the notification is linked to, but it doesn't actually effect where the email goes because the gpx_success mailer determines the recipient itself. Is that going to be confusing?

[pablobm]

Uh, you are right. The logic in the mailer should change to use recipient. Done now.

[tomhughes]

This needed because the perform_now only runs the trace import job and we need to then run the event job before the mail is submitted now?

[pablobm]

Correct. For completeness:

• The notification jobs launched by TraceImporterJob are now async (this PR moves them from deliver to deliver_later).
• The notification jobs in turn launch mailer jobs that are also async.

So the queue needs to be exhausted of the immediate jobs and those generated in turn.

[pablobm]

(EDIT 2026-04-07: comment superseded by my next one).

Good points, I should have stopped to think a bit more.

I had a look this morning, I see a couple of solutions:

• Have the record be the user instead of the trace.
  • This is what I've gone for at the moment, as it seemed simpler, but perhaps it's contrived.
• Use ephemeral notifications, which might be a more "pure" solution here. Notes:
  • With these, the initial delivery must be synchronous (no deliver_later). Note that the individual notifications (emails, etc) are still async/queued.
  • When it comes to on-site notifications, these would definitely require a separate model. That might be a good idea anyway; the proof of concept I put together last time (master...pablobm:notifications) had plenty room for improvement.
  • I have experimented with this option at pablobm/openstreetmap-website@convert-mailers-to-notifiers...pablobm:openstreetmap-website:ephemeral-notifications

Any preferences?

I have rewritten the PR description to reflect the latest changes, as well as answered the questions on specific changes.

[pablobm]

I had another look. There's a third way to do this which I now prefer. The following list supersedes the one above:

1. (New) Don't link a record. Noticed seems to work well with this and feels the most natural to me.
   • record_type and record_id are nil/NULL.
   • This requires that we pass a recipient explicitly to #deliver_later.
2. Have the record be the user instead of the trace.
   • I think this is a bit contrived.
3. Use ephemeral notifications:
   • With these, the initial delivery must be synchronous (no deliver_later). Note that the individual notifications (emails, etc) are still async/queued.
   • This also requires passing the recipient explicitly (to #deliver in this case).
   • When it comes to on-site notifications, these would definitely require a separate model. That might be a good idea anyway; the proof of concept I put together last time (master...pablobm:notifications) had plenty room for improvement.
   • I have experimented with this option at master...pablobm:ephemeral-notifications

In common to all options, any additional arguments are stored in the params column, where we can put the trace data before deletion.

• One potential issue is that the params column can get big as params[:error] may store an exception backtrace.
• When rendering later (eg: on-site notifications), the data can be accessed at params[:trace_name] and params[:trace_description] to describe the notification.

Of the options above, this PR now implements option 1. I have also integrated this notification into my proof of concept at master...pablobm:notifications. This is currently deployed to my Dev Server setup, where it can be seen in action in the dashboard of user mapper (password password):