fixes #9873 - generate unique alert mails for each user group member #2314
Conversation
| @@ -30,6 +31,8 @@ def deliver | |||
| end | |||
|
|
|||
| def group_mail(users, options) | |||
| ActiveSupport::Deprecation.warn '#group_mail is replaced by MailNotification#deliver with :users, this does not function properly' | |||
There was a problem hiding this comment.
Might be clearer to say something like :users in the options hash
|
It's nice to see someone more experienced in Rails touch my code 😄 It was useful to read through and see how you did things. Doing the locale in a block didn't occur to me. One concern I do have, and it's kind of a big one, is Katello does some data processing in the mailers: https://github.com/Katello/katello/blob/master/app/mailers/katello/errata_mailer.rb#L26 Before, with the group_mail, the data was only gathered once, but now it's going be gathered each time for each user, that could be quite an overhead for dealing with Katello's bigger datasets. Thoughts on that? I would say for statistics it'd be fine to calculate it in the async action and pass to the mailer, but the errata summaries have up to 100 errata per mail, not sure I want to pass around such big data structures in foreman tasks many times, seems inefficient. |
None really, you need to figure out which makes most sense for your usage. Per-user data should be generated in the mailer so it's specific. |
…up member To create distinct mails, new Mailer instances are required instead of using the same one - else, the last message changes the previous ones. The recipient list is now determined in the ReportImporter, and the MailNotification helps create Mailers for each recipient. The locale is also set correctly for each recipient this way, ensuring their subject lines are now also translated.
Katello (and other plugins could as well) have data sets that are not user-specific, but need to be calculated. For a dozen users subscribed to repo sync notification, now we're going to run 12x the SQL queries we would have otherwise, or have to send a big chunk of data through foreman tasks to the mailer. Maybe the latter is OK, but it'd be nicer if there was some solution to this in Foreman. |
|
I think your problem lies with foreman-tasks and not Foreman, since Foreman doesn't restrict what can be passed into a mailer, which could be your whole dataset if calculating it outside. |
|
I guess it's fine then, tasks can call the mailer with the objects, just would've rather kept the logic all in one place |
|
Might be nice, but I can't see how that's possible, mailers need to be unique or it just doesn't work reliably. I considered, and tried cloning them, but it wasn't the correct way to fix the bug. |
| mailer.constantize.send(method, *args, options.merge(:user => user)).deliver | ||
| end | ||
| else | ||
| mailer.constantize.send(method, *args).deliver |
There was a problem hiding this comment.
I think it would be a bit more readable to return early rather than make the method an "if-else":
def deliver(options)
return mailer.constantize.send(method, *args).deliver unless args.last.is_a?(Hash) && args.last.has_key?(:users)
options = args.pop
options.delete(:users).each do |user|
mailer.constantize.send(method, *args, options.merge(:user => user)).deliver
end
endThere was a problem hiding this comment.
I can't say I find that any more readable because of the long line and conditional, sorry! It might only have to be refactored back later if the usual code path (the mail.constantize..) gets any more complex..
(cherry picked from commit eabbbeb)
To create distinct mails, new Mailer instances are required instead of using
the same one - else, the last message changes the previous ones. The recipient
list is now determined in the ReportImporter, and the MailNotification helps
create Mailers for each recipient.
The locale is also set correctly for each recipient this way, ensuring their
subject lines are now also translated.