From 65ad333a58d276be73bc39eba70a9a7692a87ae6 Mon Sep 17 00:00:00 2001 From: Dominic Cleal Date: Mon, 13 Apr 2015 08:48:46 +0100 Subject: [PATCH] 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 | 11 +++++- app/mailers/host_mailer.rb | 45 ++++++++++------------- app/mailers/user_mailer.rb | 12 +++--- app/models/mail_notification.rb | 16 +++++--- app/models/user.rb | 2 +- app/services/report_importer.rb | 13 +++++-- test/unit/application_mailer_test.rb | 13 +++---- test/unit/host_mailer_test.rb | 17 +++++---- test/unit/mail_notification_test.rb | 10 +++++ test/unit/report_importer_test.rb | 55 +++++++++++++++++++--------- 10 files changed, 120 insertions(+), 74 deletions(-) diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index ed9e528c6e8..8b91d21609d 100644 --- a/app/mailers/application_mailer.rb +++ b/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 diff --git a/app/mailers/host_mailer.rb b/app/mailers/host_mailer.rb index 6c6206f0860..0b388c91957 100644 --- a/app/mailers/host_mailer.rb +++ b/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 diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index ddddc65bc04..a088cb62477 100644 --- a/app/mailers/user_mailer.rb +++ b/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 diff --git a/app/models/mail_notification.rb b/app/models/mail_notification.rb index d82b841b5c2..51c49ab6266 100644 --- a/app/models/mail_notification.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 992665f2ce6..2b653babd08 100644 --- a/app/models/user.rb +++ b/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) diff --git a/app/services/report_importer.rb b/app/services/report_importer.rb index 6bd500963c6..d757c75dd68 100644 --- a/app/services/report_importer.rb +++ b/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 diff --git a/test/unit/application_mailer_test.rb b/test/unit/application_mailer_test.rb index 2d4fec3fadd..6623bd4bfe3 100644 --- a/test/unit/application_mailer_test.rb +++ b/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 => 'nobody@example.com', :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 diff --git a/test/unit/host_mailer_test.rb b/test/unit/host_mailer_test.rb index 4964bec1eb2..2b18821e00c 100644 --- a/test/unit/host_mailer_test.rb +++ b/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 diff --git a/test/unit/mail_notification_test.rb b/test/unit/mail_notification_test.rb index 2635581b734..6bb6e18a378 100644 --- a/test/unit/mail_notification_test.rb +++ b/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 diff --git a/test/unit/report_importer_test.rb b/test/unit/report_importer_test.rb index 1f4f95a88db..efe13b3d321 100644 --- a/test/unit/report_importer_test.rb +++ b/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')