Send individual notification mails per mail recipient
With this patch series, we are introducing the general concept to send multiple mails per notification event. Specifically, we are sending one mail per recipient user. Each of those mails is rendered in the context of the recipient (i.e. with
User.current set to the recipient). The mails can be sent immediately or optionally delayed using ActiveJob.
This approach has a couple of advantages over the existing approach of sending a single mail to multiple recipients:
- We ensure object visibility for the rendered mails in the most generic way. Since each mail is rendered with
User.currentset to the recipient, all content rendered in the mail (including shown issue attributes, macros and links in rendered textile text, ...) is guaranteed to only be visible to the user without having to ensure this manually for each mail using the same visibility rules as used for the HTML views already.
- The mail will always be sent in the recipients language, even if multiple users with different languages are notified
- There is no risk of the whole notification mail being rejected because of a single invalid recipient address (see #8157, #6829, #8733 and a lot of duplicates)
In the long run, it will also allow us to introduce user-specific additional information in the rendered notification, e.g. related issues, links to assign the issue, ...
With this new approach, we do have a some challenges which we are trying to address separately:
- Since each mail is always rendered with an adapted
User.current, any mail views which expected to use the original
User.currentneed to be adapted. In Redmine core, this only affects the security notifications.
- Depending on the number of recipients for a notification, there might be many rendered mails which can take some time in the request. To ensure a snappy response, we can use ActiveJob to render and send the mails in the background. This can make the
async_*delivery methods obsolete.
Reminder: How mail sending works in plain Rails (and Redmine)¶
This patch series changes how mails are generated and delivered. Let's first look at how a mail normally gets generated and delivered in ActionMailer.
mail = Mailer.news_added(news)- The user calls a class-method of the Mailer class (a child-class of ActionMailer::Base). This method is usually not implemented in the
Mailerclass itself. Instead, there is a
ActionMailer::Base. Here, ActionMailer creates a
ActionMailer::MessageDeliveryobject. Which is basically a delegator to an eventually rendered mail object. Now, the mail is not actually rendered here. The
MessageDeliveryobject only collects all arguments it needs to eventually render the mail. This happens implicitly when any method on the mail is accessed (e.g. when the mail is about to be delivered). The
MessageDeliverydelegator is returned to the user.
Now that we have the (delegated) mail, we can deliver it with
mail.deliver. This method used to be the only delivery method. With newer Rails versions, there is (with some variations)
deliver_later. Redmine usually calls
deliver which in source:trunk/config/initializers/10-patches.rb#L160 calls
mail.deliver, the delegator starts to render the mail:
MessageDelivery#processed_mailercreates a new
Mailerinstance and calls
Mailer#process(:news_added, news)- The
processmethod is part of ActionMailer (and in turn part of the
ActionControllerframework on which ActionMailer is based on). Among things, it calls the specified action, i.e. the instance method.
Mailer#news_added(news)- This instance method is implemented on the Mailer class and is responsible to render the actual mail message. Here, we set headers, instance variables for the view and basically do the usual controller stuff. With a final call to
- This returned
Mail::Messageobject is delivered with a call to
How we hook into this process¶
Now, in order to send multiple mails, we hook into this process to render (and deliver) multiple mails. We want to ensure two things:
- We want to create multiple mails, one per recipient and deliver them all in one go.
- When rendering the mail (i.e. when calling the Mailer's instance method), we want to set
User.currentto the recipient user and set it back to the original value after the rendering is done
These two goals are achieved with two specific changes to our Mailer:
We introduce a wrapper class called
Mailer::MultiMessage. Instances of this class wrap multiple
Mail::Message objects (or rather
ActionMailer::MessageDelivery objects which delegate to
Mail::Message objects). The class provides the same
deliver_* methods as the original
Mail::Message objects. When calling such a method, we are calling it on each of the wrapped messages. This allows us to use an instance of this class in place of a
We use this class by implementing class methods for each mailer action. These class methods implement the same external interface (i.e. accept the same arguments) as the old instance methods. This ensures that we keep the existing public interface. The class methods fetch the recipient users for the respective object (i.e. the issue, news, comment, ...). We use
Mailer::MultiMessage#for to add an
ActionMailer::MessageDelivery object for each recipient which eventually renders the mail.
When creating the delegator, we also pass the recipient user to the arguments for the mail rendering. We use this user in the overwritten
Mailer#process method to set
User.current and their language before actually rendering the message. This convention allows us to keep the interfaces clear: the user is only passed between the class method and
Mailer#process but doesn't need to be specified on the instance action. Here, it is already set as
User.current for each rendered mail.
We also extend
Mailer.method_missing (on the class) to create a single-mail
Mailer::MultiMessage object with the current user in case the class method for an action is not explicitly overwritten. This ensures that our convention of always passing a user to
Mailer#process is kept up even if a plugin only adds an instance method (which was valid before).
What we tried and did not work¶
I first attempted to set the current user directly in the action's class method. and reset it directly afterwards. That would have made the code much more localized. However, due to the delayed rendering of the mail with the
ActionMailer::MessageDelivery object, the mail will be rendered only after we have already reset the current user.
One way to work around this would be to introduce
deliver_* methods (like the now obsolete
Mailer.deliver_issue_add method). However doing so would have changed the intended external interface of our
Mailer class and would likely lead to integration pain with all existing Redmine plugins sending mail.
What we ship¶
We ship three patches here:
0001-Send-individual-emails-for-each-mail-recipient.patch- The main implementation including tests.
0002-Cleanup-Remove-Issue-each_notification-and-Journal-e.patch- Removes unneccessary methods for grouping recipients based on custom field visibility
0003-Optional-Ensure-that-ActiveRecord-Base-objects-are-f.patch- An optional patch which changes the serialization behavior when sending mails with ActiveJob (e.g. with
Mailer.news_added(news).deliver_later). By default, Rails serialized the arguments as a globalid (which is basically a reference to the object in the database identified by its ID). If there are concurrent changes to e.g. a newly created issue, this might result in notifications using the new state instead of the original state. With this patch, we serialize the exact attributes of the given objects into the job. Since this also causes nested attributes to be serialized along, care must be taken to avoid reference errors. In general, we should probably try to use mostly immutable state in the database where it is an important consideration that the notification state is exactly shown in the DB.
If the general direction of this patch is accepted, it is probably desireable to move the mail sending to ActiveJob completely. This could replace the
async_* delivery methods currently configurable in
configuration.yml. This change can either be hardcoded in source:trunk/config/initializers/10-patches.rb#L160 or made configurable with a setting. I'll be glad to provide a patch for this. It would only be a few lines of code.
By default, we could use the in-process ActiveJob runner to send mails. More advanced users with a dedicated job worker (e.g. delayed job) can easily switch to that without changes to the Redmine code itself.
Since each user gets their own mail, we can probably remove the setting to send mails with BCC completely. If plugins want to send mails to non-user recipients, we would have to make sure to either set the recipients manually on BCC or group them appropriately. It is not a required setting in Redmine core anymore though.