Feature #26791
closed
Send individual notification mails per mail recipient
0%
Description
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 originalUser.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 theMailerclass itself. Instead, there is amethod_missingmethod onActionMailer::Base. Here, ActionMailer creates aActionMailer::MessageDeliveryobject. Which is basically a delegator to an eventually rendered mail object. Now, the mail is not actually rendered here. TheMessageDeliveryobject 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). TheMessageDeliverydelegator 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_now and deliver_later. Redmine usually calls deliver which in source:trunk/config/initializers/10-patches.rb#L160 calls deliver_now currently.
When calling mail.deliver, the delegator starts to render the mail:
MessageDelivery#processed_mailercreates a newMailerinstance and callsprocesson it.Mailer#process(:news_added, news)- Theprocessmethod is part of ActionMailer (and in turn part of theActionControllerframework 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 tomail, we eventually render a single mail and return aMail::Messageobject.- This returned
Mail::Messageobject is delivered with a call toMail::Message#deliver_now.
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 ActionMailer::MessageDelivery object.
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 visibility0003-Optional-Ensure-that-ActiveRecord-Base-objects-are-f.patch- An optional patch which changes the serialization behavior when sending mails with ActiveJob (e.g. withMailer.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.
Future work¶
ActiveJob sending¶
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.
BCC recipients¶
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.
Files
Related issues
Updated by 
Updated by 
Updated by Toshi MARUYAMA 
Updated by 
Updated by 
Updated by 
Updated by 
Updated by 
Updated by 
Updated by