Patch #26791

Send individual notification mails per mail recipient

Added by Holger Just 3 months ago. Updated 17 days ago.

Status:NewStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:Email notifications
Target version:4.1.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.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.

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 Mailer class itself. Instead, there is a method_missing 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 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.

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 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 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.

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.

0002-Cleanup-Remove-Issue-each_notification-and-Journal-e.patch Magnifier (1.87 KB) Holger Just, 2017-08-30 12:46

0001-Send-individual-emails-for-each-mail-recipient.patch Magnifier (53.1 KB) Holger Just, 2017-08-30 12:46

0003-Optional-Ensure-that-ActiveRecord-Base-objects-are-f.patch Magnifier (1.88 KB) Holger Just, 2017-08-30 12:46


Related issues

Related to Redmine - Defect #8157: Redmine do not send notification emails if a recipients e... New
Related to Redmine - Defect #11981: redmine send mail fail when project has too much members New
Related to Redmine - Feature #5990: Email notification should reflect user language setting New 2010-07-29
Related to Redmine - Defect #17096: Issue mails cannot be threaded by some mailers New
Related to Redmine - Defect #27242: issue creation failed if email notification fail Closed
Related to Redmine - Defect #11106: Email notification processing is slow - Redmine 1.2.2 Closed
Blocks Redmine - Feature #2496: Per user email format settings (HTML/plain text) New 2009-01-13

History

#2 Updated by Jan from Planio www.plan.io 3 months ago

  • Related to Defect #8157: Redmine do not send notification emails if a recipients email address is not valid added

#3 Updated by Go MAEDA 3 months ago

  • Related to Defect #11981: redmine send mail fail when project has too much members added

#4 Updated by Go MAEDA 3 months ago

  • Related to Feature #5990: Email notification should reflect user language setting added

#5 Updated by Go MAEDA 3 months ago

  • Blocks Feature #2496: Per user email format settings (HTML/plain text) added

#6 Updated by Go MAEDA 2 months ago

  • Related to Defect #17096: Issue mails cannot be threaded by some mailers added

#7 Updated by Toshi MARUYAMA about 1 month ago

  • Description updated (diff)

#9 Updated by Go MAEDA about 1 month ago

  • Related to Defect #27242: issue creation failed if email notification fail added

#10 Updated by Go MAEDA 17 days ago

  • Target version set to 4.1.0

This patch can fix many annoying behaviors related to email notification (see "Related issues"). Setting target version to 4.1.0.

#11 Updated by Go MAEDA 17 days ago

  • Related to Defect #11106: Email notification processing is slow - Redmine 1.2.2 added

Also available in: Atom PDF