Feature #26791
Updated by Toshi MARUYAMA over 6 years ago
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.current@ set 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.current@ need 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. absolete. h2. 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 deliverd 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 @Mailer@ class itself. Instead, there is a "@method_missing@":https://github.com/rails/rails/blob/v5.1.2/actionmailer/lib/action_mailer/base.rb#L576-L582 method on @ActionMailer::Base@. Here, ActionMailer creates a @ActionMailer::MessageDelivery@ object. Which is basically a delegator to an _eventually rendered_ mail object. Now, the mail is not actually rendered here. The @MessageDelivery@ object 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 @MessageDelivery@ delegator 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_mailer@":https://github.com/rails/rails/blob/v5.1.3/actionmailer/lib/action_mailer/message_delivery.rb#L103-L107 creates a new @Mailer@ instance and calls @process@ on it. * @Mailer#process(:news_added, news)@ - The @process@ method is part of ActionMailer (and in turn part of the @ActionController@ framework 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 @mail@, we eventually render a single mail and return a @Mail::Message@ object. * This returned @Mail::Message@ object is delivered with a call to @Mail::Message#deliver_now@. h2. 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.current@ to the recipient user and set it back to the original value after the rendering is done These two goals are achieved with two specific specifc 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). h2. 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. h2. 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. h2. Future work h3. 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 [[EmailConfiguration#Asynchronous-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. h3. 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.