diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index e94f6d1e5ef..913c2888a96 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 in the options hash, this does not function properly' + mails = users.map do |user| @user = user set_locale_for user @@ -40,7 +43,13 @@ 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 def set_url diff --git a/app/mailers/audit_mailer.rb b/app/mailers/audit_mailer.rb index a9bce1daa6d..e18261a530b 100644 --- a/app/mailers/audit_mailer.rb +++ b/app/mailers/audit_mailer.rb @@ -5,15 +5,13 @@ def summary(options = {}) raise ::Foreman::Exception.new(N_("Must specify a user with email enabled")) unless (user=User.find(options[:user])) && user.mail_enabled? time = options[:time] ? %(time >= "#{options[:time]}") : 'time > yesterday' set_url - set_locale_for(user) @query = options[:query].present? ? "#{options[:query]} and #{time}" : "#{time}" @count = Audit.authorized_as(user, :view_audit_logs, Audit).search_for(@query).count @limit = Setting[:entries_per_page] > @count ? @count : Setting[:entries_per_page] @audits = Audit.authorized_as(user, :view_audit_logs, Audit).search_for(@query).limit(@limit) - @subject = _("Audit summary") - mail(:to => user.mail, - :from => Setting["email_reply_address"], - :subject => @subject) + set_locale_for(user) do + mail(:to => user.mail, :subject => _("Audit summary")) + end end end diff --git a/app/mailers/host_mailer.rb b/app/mailers/host_mailer.rb index 0a5bda2b311..3afed513399 100644 --- a/app/mailers/host_mailer.rb +++ b/app/mailers/host_mailer.rb @@ -14,39 +14,32 @@ 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) + 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 diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index ddddc65bc04..59b6deee993 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -2,13 +2,11 @@ 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")) + end end end diff --git a/app/models/mail_notification.rb b/app/models/mail_notification.rb index 9c1f07bf031..f4db6c0be42 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 76ba3106317..c299ef31be1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -401,7 +401,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')