fixes #9873 - generate unique alert mails for each user group 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.

app/mailers/application_mailer.rb

@@ -15,6 +15,7 @@ def mail(headers = {}, &block)
 
   class GroupMail
     def initialize(emails)
+      ActiveSupport::Deprecation.warn 'GroupMail will be removed as mailers should not generate multiple messages, use MailNotification#deliver'
       @emails = emails
     end
 
@@ -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'
+
     mails = users.map do |user|
       @user = user
       set_locale_for user
@@ -40,6 +43,12 @@ def group_mail(users, options)
   end
 
   def set_locale_for(user)
-    FastGettext.set_locale(user.locale.blank? ? "en" : user.locale)
+    old_loc = FastGettext.locale
+    begin
+      FastGettext.set_locale(user.locale.blank? ? 'en' : user.locale)
+      yield if block_given?
+    ensure
+      FastGettext.locale = old_loc if block_given?
+    end
   end
 end

app/mailers/host_mailer.rb

@@ -14,46 +14,41 @@ def summary(options = {})
     total = 0
     total_metrics.values.each { |v| total += v }
 
-    subject = _("Puppet Summary Report - F:%{failed} R:%{restarted} S:%{skipped} A:%{applied} FR:%{failed_restarts} T:%{total}") % {
-      :failed => total_metrics["failed"],
-      :restarted => total_metrics["restarted"],
-      :skipped => total_metrics["skipped"],
-      :applied => total_metrics["applied"],
-      :failed_restarts => total_metrics["failed_restarts"],
-      :total => total
-    }
-
     @hosts = host_data
     @timerange = time
     @out_of_sync = hosts.out_of_sync
     @disabled = hosts.alerts_disabled
-
     set_url
-    set_locale_for user
 
-    mail(:to   => user.mail,
-         :from => Setting["email_reply_address"],
-         :subject => subject,
-         :date => Time.zone.now )
+    set_locale_for(user) do
+      subject = _("Puppet Summary Report - F:%{failed} R:%{restarted} S:%{skipped} A:%{applied} FR:%{failed_restarts} T:%{total}") % {
+        :failed => total_metrics["failed"],
+        :restarted => total_metrics["restarted"],
+        :skipped => total_metrics["skipped"],
+        :applied => total_metrics["applied"],
+        :failed_restarts => total_metrics["failed_restarts"],
+        :total => total
+      }
+
+      mail(:to   => user.mail,
+           :subject => subject,
+           :date => Time.zone.now )
+    end
   end
 
-  def error_state(report)
-    report = Report.find(report)
-    host = report.host
-    owners = host.owner.recipients_for(:puppet_error_state) if host.owner.present?
-    owners ||= []
-    raise ::Foreman::Exception.new(N_("unable to find recipients")) if owners.empty?
+  def error_state(report, options = {})
     @report = report
-    @host = host
-
-    group_mail(owners, :subject => (_("Puppet error on %s") % host.to_label))
+    @host = @report.host
+    set_locale_for(options[:user]) do
+      mail(:to => options[:user].mail, :subject => (_("Puppet error on %s") % @host.to_label))
+    end
   end
 
   private
 
   def set_url
     unless (@url = URI.parse(Setting[:foreman_url])).present?
-      raise ":foreman_url is not set, please configure in the Foreman Web UI (More -> Settings -> General)"
+      raise ::Foreman::Exception.new(N_('foreman_url is not set, please configure under Administer > Settings > General'))
     end
   end
 

app/mailers/user_mailer.rb

@@ -2,13 +2,13 @@ class UserMailer < ApplicationMailer
   helper :reports
 
   def welcome(options = {})
-    user = User.find(options[:user])
+    user = options[:user]
     @login = user.login
 
-    set_locale_for user
-
-    mail(:to      => user.mail,
-         :subject => _("Welcome to Foreman"),
-         :date    => Time.zone.now)
+    set_locale_for(user) do
+      mail(:to      => user.mail,
+           :subject => _("Welcome to Foreman"),
+           :date    => Time.zone.now)
+    end
   end
 end

app/models/mail_notification.rb

@@ -31,10 +31,16 @@ def self.[](name)
     self.find_by_name(name)
   end
 
-  def deliver(options)
-    mailer.constantize.send(method, options).deliver
-  rescue => e
-    logger.warn "Failed to send email notification #{name}: #{e}"
-    logger.debug e.backtrace
+  def deliver(*args)
+    # args can be anything really, treat it carefully
+    # handle args=[.., :users => [..]] specially and instantiate a single mailer per user, with :user set on each
+    if 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
+    else
+      mailer.constantize.send(method, *args).deliver
+    end
   end
 end

app/models/user.rb

@@ -396,7 +396,7 @@ def prepare_password
 
   def welcome_mail
     return unless mail_enabled? && internal? && Setting[:send_welcome_email]
-    MailNotification[:welcome].deliver(:user => self.id)
+    MailNotification[:welcome].deliver(:user => self)
   end
 
   def encrypt_password(pass)

app/services/report_importer.rb

@@ -99,10 +99,15 @@ def inspect_report
     if report.error?
       # found a report with errors
       # notify via email IF enabled is set to true
-      logger.warn "#{name} is disabled - skipping." and return if host.disabled?
-
-      logger.debug 'error detected, checking if we need to send an email alert'
-      MailNotification[:puppet_error_state].deliver(report.id)
+      logger.warn "#{name} is disabled - skipping alert" and return if host.disabled?
+
+      owners = host.owner.present? ? host.owner.recipients_for(:puppet_error_state) : []
+      if owners.present?
+        logger.debug "sending alert to #{owners.map(&:login).join(',')}"
+        MailNotification[:puppet_error_state].deliver(report, :users => owners)
+      else
+        logger.debug "no owner or recipients for alert on #{name}"
+      end
     end
   end
 end

test/unit/application_mailer_test.rb

@@ -1,22 +1,21 @@
 require 'test_helper'
 
 class ApplicationMailerTest < ActiveSupport::TestCase
-  setup do
-    ActionMailer::Base.deliveries = []
-    Setting[:email_subject_prefix] = '[foreman-production]'
+  setup { ActionMailer::Base.deliveries = [] }
 
+  def mail
     ApplicationMailer.mail(:to => '[email protected]', :subject => 'Danger, Will Robinson!') do |mail|
       format.text "This is a test mail."
     end.deliver
-
-    @mail = ActionMailer::Base.deliveries.first
+    ActionMailer::Base.deliveries.last
   end
 
   test "foreman server header is set" do
-    assert_equal @mail.header['X-Foreman-Server'].to_s, 'foreman.some.host.fqdn'
+    assert_equal mail.header['X-Foreman-Server'].to_s, 'foreman.some.host.fqdn'
   end
 
   test "foreman subject prefix is attached" do
-    assert_match /^\[foreman-production\]/, @mail.subject
+    Setting[:email_subject_prefix] = '[foreman-production]'
+    assert_match /^\[foreman-production\]/, mail.subject
   end
 end

test/unit/host_mailer_test.rb

@@ -25,6 +25,8 @@ def setup
     # we do it here:
     Environment.reset_counters(@env, :hosts)
     @env.reload
+
+    ActionMailer::Base.deliveries = []
   end
 
   test "mail should have the specified recipient" do
@@ -49,12 +51,13 @@ def setup
     assert HostMailer.summary(@options).deliver.body.include?(@host.name)
   end
 
-  test "error state raises exception if host has no owners" do
-    host = stub(:host)
-    host.stubs(:owner)
-    report = stub(:report)
-    report.stubs(:host).returns(host)
-    Report.stubs(:find).returns(report)
-    assert_raise(Foreman::Exception) { HostMailer.error_state(1).deliver }
+  test 'error_state sends mail with correct headers' do
+    report = FactoryGirl.create(:report)
+    user = FactoryGirl.create(:user, :with_mail)
+    mail = HostMailer.error_state(report, :user => user).deliver
+    assert_includes mail.from, Setting["email_reply_address"]
+    assert_includes mail.to, user.mail
+    assert_includes mail.subject, report.host.name
+    assert_present mail.body
   end
 end

test/unit/mail_notification_test.rb

@@ -15,4 +15,14 @@ class MailNotificationTest < ActiveSupport::TestCase
       notification.deliver
     end
   end
+
+  test "#deliver generates mails for each user in :users option" do
+    users = FactoryGirl.create_pair(:user, :with_mail)
+    mailer = FactoryGirl.create(:mail_notification)
+    mail = mock('mail')
+    mail.expects(:deliver).twice
+    HostMailer.expects(:test_mail).with(:foo, :user => users[0]).returns(mail)
+    HostMailer.expects(:test_mail).with(:foo, :user => users[1]).returns(mail)
+    mailer.deliver(:foo, :users => users)
+  end
 end

test/unit/report_importer_test.rb

@@ -1,15 +1,8 @@
 require 'test_helper'
 
 class ReportImporterTest < ActiveSupport::TestCase
-  def setup
-    User.current = users :admin
+  setup do
     ActionMailer::Base.deliveries = []
-    @owner = FactoryGirl.create(:user, :admin, :with_mail)
-    @host = FactoryGirl.create(:host, :owner => @owner)
-  end
-
-  def teardown
-    User.current = nil
   end
 
   test 'json_fixture_loader' do
@@ -27,24 +20,50 @@ def teardown
     assert_empty r.logs
   end
 
-  test 'when owner is subscribed to notification, a mail should be sent on error' do
-    @owner.mail_notifications << MailNotification[:puppet_error_state]
-    assert_difference 'ActionMailer::Base.deliveries.size' do
-      report = read_json_fixture('report-errors.json')
-      report["host"] = @host.name
-      ReportImporter.import report
+  context 'with user owner' do
+    setup do
+      @owner = as_admin { FactoryGirl.create(:user, :admin, :with_mail) }
+      @host = FactoryGirl.create(:host, :owner => @owner)
+    end
+
+    test 'when owner is subscribed to notification, a mail should be sent on error' do
+      @owner.mail_notifications << MailNotification[:puppet_error_state]
+      assert_difference 'ActionMailer::Base.deliveries.size' do
+        report = read_json_fixture('report-errors.json')
+        report["host"] = @host.name
+        ReportImporter.import report
+      end
+    end
+
+    test 'when owner is not subscribed to notifications, no mail should be sent on error' do
+      @owner.mail_notifications = []
+      assert_no_difference 'ActionMailer::Base.deliveries.size' do
+        report = read_json_fixture('report-errors.json')
+        report["host"] = @host.name
+        ReportImporter.import report
+      end
     end
   end
 
-  test 'when owner is not subscribed to notifications, no mail should be sent on error' do
-    @owner.mail_notifications = []
+  test 'when a host has no owner, no mail should be sent on error' do
+    host = FactoryGirl.create(:host, :owner => nil)
+    report = read_json_fixture('report-errors.json')
+    report["host"] = host.name
     assert_no_difference 'ActionMailer::Base.deliveries.size' do
-      report = read_json_fixture('report-errors.json')
-      report["host"] = @host.name
       ReportImporter.import report
     end
   end
 
+  test 'when usergroup owner is subscribed to notification, a mail should be sent to all users on error' do
+    ug = FactoryGirl.create(:usergroup, :users => FactoryGirl.create_pair(:user, :with_mail))
+    Usergroup.any_instance.expects(:recipients_for).with(:puppet_error_state).returns(ug.users)
+    host = FactoryGirl.create(:host, :owner => ug)
+    report = read_json_fixture('report-errors.json')
+    report["host"] = host.name
+    ReportImporter.import report
+    assert_equal ug.users.map { |u| u.mail }, ActionMailer::Base.deliveries.map { |d| d.to }.flatten
+  end
+
   test 'if report has no error, no mail should be sent' do
     assert_no_difference 'ActionMailer::Base.deliveries.size' do
       ReportImporter.import read_json_fixture('report-applied.json')