Defect #44300
open
Mailer.with_deliveries(false) does not prevent emails from being sent
Description
While investigating #44299 on a development instance where email delivery is disabled, and with the "Send email notifications during the project copy" box left unchecked, I noticed that the emails were pushed to the job queue nevertheless.
Mailer.with_deliveries only flips ActionMailer::Base.perform_deliveries, and that flag is read when the mail is delivered. Since Redmine sends everything with deliver_later, the Mailer::DeliveryJob runs after the block has already restored the flag or, with a queue backend, in a process where it was never false at all. So the mails are enqueued and sent.
The only caller may be the "Send email notifications during the project copy" checkbox of ProjectsController#copy. Unchecking it does not prevent anything: copying a project, I get one Mailer::DeliveryJob enqueued per copied issue, and the worker delivers them.
Proposed patch: skip the enqueuing itself.
class DeliveryJob < ActionMailer::MailDeliveryJob
before_enqueue {throw :abort unless ActionMailer::Base.perform_deliveries}
This makes with_deliveries(false) effective whatever the queue adapter, and it also stops piling up delivery jobs that are discarded at delivery time on instances where email is not configured (perform_deliveries defaults to false in config/application.rb).
Files
Updated by Go MAEDA 21 days ago
- File mailer_with_deliveries_enqueue.patch mailer_with_deliveries_enqueue.patch added
- Target version set to 6.1.4
The attached patch could not be applied with git apply or patch, so I am attaching a regenerated one with the same content.
Setting the target version to 6.1.4.
Updated by Holger Just 20 days ago
Note that Mailer.with_deliveries is generally not thread-safe (since it sets the global ActionMailer::Base.perform_deliveries attribute). As such, when running Redmine with a multi-threaded server, using this method may cause mails to be lost in other threads.
Accordingly, I believe Mailer.with_deliveries should rather set a thread-local variable in stead of setting the global ActionMailer::Base.perform_deliveries flag. This new variable can then be checked in the before_enqueue hook in addition to the global ActionMailer::Base.perform_deliveries flag (which should not change dynamically). This new variable could e.g. be stored in RequestStore.store[:mailer_perform_deliveries].