From: Eric Davis <edavis@littlestreamsoftware.com>
Date: Sat, 28 Mar 2009 00:38:57 +0000 (+0000)
Subject: Added observers to watch model objects for mail delivery instead of calling Mailer.
X-Git-Tag: 0.9.0~545
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=b4be8849c0de81841c458c0f059787a9cc9bc022;p=redmine.git

Added observers to watch model objects for mail delivery instead of calling Mailer.

* Added an IssueObserver to watch when Issues are created
* Added a JournalObserver to watch when Journals are created (Issue updates)
* Added a NewsObserver for News items.
* Added a DocumentObserver for Document notifications.
* Setup IssuesController#new to use the IssueObserver.
* Setup IssuesController#edit to use the IssueObserver.
* Setup IssuesController#bulk_edit to use the JournalObserver.
* Removed the Mailer call in Changeset#scan_commit_for_issue_ids, the
  JournalObserver will handle it.
* Removed Mailer calls in MailHandler in favor of the Observers.

  #2659

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@2637 e93f8b46-1217-0410-a6f0-8f06a7374b81
---

diff --git a/app/controllers/documents_controller.rb b/app/controllers/documents_controller.rb
index 965614149..f5825e7c3 100644
--- a/app/controllers/documents_controller.rb
+++ b/app/controllers/documents_controller.rb
@@ -48,7 +48,6 @@ class DocumentsController < ApplicationController
     if request.post? and @document.save	
       attach_files(@document, params[:attachments])
       flash[:notice] = l(:notice_successful_create)
-      Mailer.deliver_document_added(@document) if Setting.notified_events.include?('document_added')
       redirect_to :action => 'index', :project_id => @project
     end
   end
diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index 929b928ac..a41dc5040 100644
--- a/app/controllers/issues_controller.rb
+++ b/app/controllers/issues_controller.rb
@@ -146,7 +146,6 @@ class IssuesController < ApplicationController
       if @issue.save
         attach_files(@issue, params[:attachments])
         flash[:notice] = l(:notice_successful_create)
-        Mailer.deliver_issue_add(@issue) if Setting.notified_events.include?('issue_added')
         call_hook(:controller_issues_new_after_save, { :params => params, :issue => @issue})
         redirect_to(params[:continue] ? { :action => 'new', :tracker_id => @issue.tracker } :
                                         { :action => 'show', :id => @issue })
@@ -193,7 +192,6 @@ class IssuesController < ApplicationController
         if !journal.new_record?
           # Only send notification if something was actually changed
           flash[:notice] = l(:notice_successful_update)
-          Mailer.deliver_issue_edit(journal) if Setting.notified_events.include?('issue_updated')
         end
         call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => journal})
         redirect_to(params[:back_to] || {:action => 'show', :id => @issue})
@@ -247,10 +245,7 @@ class IssuesController < ApplicationController
         issue.custom_field_values = custom_field_values if custom_field_values && !custom_field_values.empty?
         call_hook(:controller_issues_bulk_edit_before_save, { :params => params, :issue => issue })
         # Don't save any change to the issue if the user is not authorized to apply the requested status
-        if (status.nil? || (issue.status.new_status_allowed_to?(status, current_role, issue.tracker) && issue.status = status)) && issue.save
-          # Send notification for each issue (if changed)
-          Mailer.deliver_issue_edit(journal) if journal.details.any? && Setting.notified_events.include?('issue_updated')
-        else
+        unless (status.nil? || (issue.status.new_status_allowed_to?(status, current_role, issue.tracker) && issue.status = status)) && issue.save
           # Keep unsaved issue ids to display them in flash error
           unsaved_issue_ids << issue.id
         end
diff --git a/app/controllers/news_controller.rb b/app/controllers/news_controller.rb
index b5f7ca1b2..9fc9f5b6a 100644
--- a/app/controllers/news_controller.rb
+++ b/app/controllers/news_controller.rb
@@ -45,7 +45,6 @@ class NewsController < ApplicationController
       @news.attributes = params[:news]
       if @news.save
         flash[:notice] = l(:notice_successful_create)
-        Mailer.deliver_news_added(@news) if Setting.notified_events.include?('news_added')
         redirect_to :controller => 'news', :action => 'index', :project_id => @project
       end
     end
diff --git a/app/models/changeset.rb b/app/models/changeset.rb
index e29de363b..41b4befc6 100644
--- a/app/models/changeset.rb
+++ b/app/models/changeset.rb
@@ -113,7 +113,6 @@ class Changeset < ActiveRecord::Base
           issue.status = fix_status
           issue.done_ratio = done_ratio if done_ratio
           issue.save
-          Mailer.deliver_issue_edit(journal) if Setting.notified_events.include?('issue_updated')
         end
       end
       referenced_issues += target_issues
diff --git a/app/models/document_observer.rb b/app/models/document_observer.rb
new file mode 100644
index 000000000..3125c97b3
--- /dev/null
+++ b/app/models/document_observer.rb
@@ -0,0 +1,22 @@
+# redMine - project management software
+# Copyright (C) 2006-2007  Jean-Philippe Lang
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+
+class DocumentObserver < ActiveRecord::Observer
+  def after_create(document)
+    Mailer.deliver_document_added(document) if Setting.notified_events.include?('document_added')
+  end
+end
diff --git a/app/models/issue_observer.rb b/app/models/issue_observer.rb
new file mode 100644
index 000000000..bdb5c1d4a
--- /dev/null
+++ b/app/models/issue_observer.rb
@@ -0,0 +1,22 @@
+# redMine - project management software
+# Copyright (C) 2006-2007  Jean-Philippe Lang
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+
+class IssueObserver < ActiveRecord::Observer
+  def after_create(issue)
+    Mailer.deliver_issue_add(issue) if Setting.notified_events.include?('issue_added')
+  end
+end
diff --git a/app/models/journal_observer.rb b/app/models/journal_observer.rb
new file mode 100644
index 000000000..5604e064e
--- /dev/null
+++ b/app/models/journal_observer.rb
@@ -0,0 +1,22 @@
+# redMine - project management software
+# Copyright (C) 2006-2007  Jean-Philippe Lang
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+
+class JournalObserver < ActiveRecord::Observer
+  def after_create(journal)
+    Mailer.deliver_issue_edit(journal) if Setting.notified_events.include?('issue_updated')
+  end
+end
diff --git a/app/models/mail_handler.rb b/app/models/mail_handler.rb
index 9b6410d57..023e1d63c 100644
--- a/app/models/mail_handler.rb
+++ b/app/models/mail_handler.rb
@@ -110,13 +110,11 @@ class MailHandler < ActionMailer::Base
       end
       h
     end
+    # add To and Cc as watchers before saving so the watchers can reply to Redmine
+    add_watchers(issue)
     issue.save!
     add_attachments(issue)
     logger.info "MailHandler: issue ##{issue.id} created by #{user}" if logger && logger.info
-    # add To and Cc as watchers
-    add_watchers(issue)
-    # send notification after adding watchers so that they can reply to Redmine
-    Mailer.deliver_issue_add(issue) if Setting.notified_events.include?('issue_added')
     issue
   end
   
@@ -148,7 +146,6 @@ class MailHandler < ActionMailer::Base
     end
     issue.save!
     logger.info "MailHandler: issue ##{issue.id} updated by #{user}" if logger && logger.info
-    Mailer.deliver_issue_edit(journal) if Setting.notified_events.include?('issue_updated')
     journal
   end
   
diff --git a/app/models/news_observer.rb b/app/models/news_observer.rb
new file mode 100644
index 000000000..e10aec55e
--- /dev/null
+++ b/app/models/news_observer.rb
@@ -0,0 +1,22 @@
+# redMine - project management software
+# Copyright (C) 2006-2007  Jean-Philippe Lang
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+
+class NewsObserver < ActiveRecord::Observer
+  def after_create(news)
+    Mailer.deliver_news_added(news) if Setting.notified_events.include?('news_added')
+  end
+end
diff --git a/config/environment.rb b/config/environment.rb
index 5aeb140de..826856d13 100644
--- a/config/environment.rb
+++ b/config/environment.rb
@@ -36,7 +36,7 @@ Rails::Initializer.run do |config|
   
   # Activate observers that should always be running
   # config.active_record.observers = :cacher, :garbage_collector
-  config.active_record.observers = :message_observer
+  config.active_record.observers = :message_observer, :issue_observer, :journal_observer, :news_observer, :document_observer
 
   # Make Active Record use UTC-base instead of local time
   # config.active_record.default_timezone = :utc
diff --git a/test/functional/documents_controller_test.rb b/test/functional/documents_controller_test.rb
index 2ad94aacf..b5788c776 100644
--- a/test/functional/documents_controller_test.rb
+++ b/test/functional/documents_controller_test.rb
@@ -66,6 +66,8 @@ class DocumentsControllerTest < Test::Unit::TestCase
   end
   
   def test_new_with_one_attachment
+    ActionMailer::Base.deliveries.clear
+    Setting.notified_events << 'document_added'
     @request.session[:user_id] = 2
     set_tmp_attachments_directory
     
@@ -82,6 +84,7 @@ class DocumentsControllerTest < Test::Unit::TestCase
     assert_equal Enumeration.find(2), document.category
     assert_equal 1, document.attachments.size
     assert_equal 'testfile.txt', document.attachments.first.filename
+    assert_equal 1, ActionMailer::Base.deliveries.size
   end
   
   def test_edit_routing
diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb
index 9dd188011..0df66abdb 100644
--- a/test/functional/issues_controller_test.rb
+++ b/test/functional/issues_controller_test.rb
@@ -503,6 +503,21 @@ class IssuesControllerTest < Test::Unit::TestCase
     assert [mail.bcc, mail.cc].flatten.include?(User.find(3).mail)
   end
   
+  def test_post_new_should_send_a_notification
+    ActionMailer::Base.deliveries.clear
+    @request.session[:user_id] = 2
+    post :new, :project_id => 1, 
+               :issue => {:tracker_id => 3,
+                          :subject => 'This is the test_new issue',
+                          :description => 'This is the description',
+                          :priority_id => 5,
+                          :estimated_hours => '',
+                          :custom_field_values => {'2' => 'Value for field 2'}}
+    assert_redirected_to :action => 'show'
+    
+    assert_equal 1, ActionMailer::Base.deliveries.size
+  end
+  
   def test_post_should_preserve_fields_values_on_validation_failure
     @request.session[:user_id] = 2
     post :new, :project_id => 1, 
@@ -760,6 +775,20 @@ class IssuesControllerTest < Test::Unit::TestCase
     # No email should be sent
     assert ActionMailer::Base.deliveries.empty?
   end
+
+  def test_post_edit_should_send_a_notification
+    @request.session[:user_id] = 2
+    ActionMailer::Base.deliveries.clear
+    issue = Issue.find(1)
+    old_subject = issue.subject
+    new_subject = 'Subject modified by IssuesControllerTest#test_post_edit'
+    
+    post :edit, :id => 1, :issue => {:subject => new_subject,
+                                     :priority_id => '6',
+                                     :category_id => '1' # no change
+                                    }
+    assert_equal 1, ActionMailer::Base.deliveries.size
+  end
   
   def test_post_edit_with_invalid_spent_time
     @request.session[:user_id] = 2
@@ -797,6 +826,22 @@ class IssuesControllerTest < Test::Unit::TestCase
     assert_equal 1, journal.details.size
   end
 
+  def test_bullk_edit_should_send_a_notification
+    @request.session[:user_id] = 2
+    ActionMailer::Base.deliveries.clear
+    post(:bulk_edit,
+         {
+           :ids => [1, 2],
+           :priority_id => 7,
+           :assigned_to_id => '',
+           :custom_field_values => {'2' => ''},
+           :notes => 'Bulk editing'
+         })
+
+    assert_response 302
+    assert_equal 2, ActionMailer::Base.deliveries.size
+  end
+
   def test_bulk_edit_custom_field
     @request.session[:user_id] = 2
     # update issues priority
diff --git a/test/functional/news_controller_test.rb b/test/functional/news_controller_test.rb
index 009e58c73..22ad2d241 100644
--- a/test/functional/news_controller_test.rb
+++ b/test/functional/news_controller_test.rb
@@ -112,6 +112,9 @@ class NewsControllerTest < Test::Unit::TestCase
   end
   
   def test_post_new
+    ActionMailer::Base.deliveries.clear
+    Setting.notified_events << 'news_added'
+
     @request.session[:user_id] = 2
     post :new, :project_id => 1, :news => { :title => 'NewsControllerTest',
                                             :description => 'This is the description',
@@ -123,6 +126,7 @@ class NewsControllerTest < Test::Unit::TestCase
     assert_equal 'This is the description', news.description
     assert_equal User.find(2), news.author
     assert_equal Project.find(1), news.project
+    assert_equal 1, ActionMailer::Base.deliveries.size
   end
   
   def test_edit_routing
diff --git a/test/unit/changeset_test.rb b/test/unit/changeset_test.rb
index 6cc53d852..6a0df2c5d 100644
--- a/test/unit/changeset_test.rb
+++ b/test/unit/changeset_test.rb
@@ -24,6 +24,7 @@ class ChangesetTest < Test::Unit::TestCase
   end
   
   def test_ref_keywords_any
+    ActionMailer::Base.deliveries.clear
     Setting.commit_fix_status_id = IssueStatus.find(:first, :conditions => ["is_closed = ?", true]).id
     Setting.commit_fix_done_ratio = '90'
     Setting.commit_ref_keywords = '*'
@@ -38,6 +39,7 @@ class ChangesetTest < Test::Unit::TestCase
     fixed = Issue.find(1)
     assert fixed.closed?
     assert_equal 90, fixed.done_ratio
+    assert_equal 1, ActionMailer::Base.deliveries.size
   end
   
   def test_ref_keywords_any_line_start
diff --git a/test/unit/document_test.rb b/test/unit/document_test.rb
index 17a0ad6ea..1950f8558 100644
--- a/test/unit/document_test.rb
+++ b/test/unit/document_test.rb
@@ -25,6 +25,15 @@ class DocumentTest < Test::Unit::TestCase
     assert doc.save
   end
   
+  def test_create_should_send_email_notification
+    ActionMailer::Base.deliveries.clear
+    Setting.notified_events << 'document_added'
+    doc = Document.new(:project => Project.find(1), :title => 'New document', :category => Enumeration.find_by_name('User documentation'))
+
+    assert doc.save
+    assert_equal 1, ActionMailer::Base.deliveries.size
+  end
+
   def test_create_with_default_category
     # Sets a default category
     e = Enumeration.find_by_name('Technical documentation')
diff --git a/test/unit/issue_test.rb b/test/unit/issue_test.rb
index 365316034..ae3fa5bda 100644
--- a/test/unit/issue_test.rb
+++ b/test/unit/issue_test.rb
@@ -241,4 +241,12 @@ class IssueTest < Test::Unit::TestCase
     assert !Issue.new(:due_date => nil).overdue?
     assert !Issue.new(:due_date => 1.day.ago.to_date, :status => IssueStatus.find(:first, :conditions => {:is_closed => true})).overdue?
   end
+  
+  def test_create_should_send_email_notification
+    ActionMailer::Base.deliveries.clear
+    issue = Issue.new(:project_id => 1, :tracker_id => 1, :author_id => 3, :status_id => 1, :priority => Enumeration.priorities.first, :subject => 'test_create', :estimated_hours => '1:30')
+
+    assert issue.save
+    assert_equal 1, ActionMailer::Base.deliveries.size
+  end
 end
diff --git a/test/unit/journal_test.rb b/test/unit/journal_test.rb
index b177f3198..147af4aae 100644
--- a/test/unit/journal_test.rb
+++ b/test/unit/journal_test.rb
@@ -36,4 +36,15 @@ class JournalTest < Test::Unit::TestCase
     assert_kind_of IssueStatus, status
     assert_equal 2, status.id 
   end
+  
+  def test_create_should_send_email_notification
+    ActionMailer::Base.deliveries.clear
+    issue = Issue.find(:first)
+    user = User.find(:first)
+    journal = issue.init_journal(user, issue)
+
+    assert journal.save
+    assert_equal 1, ActionMailer::Base.deliveries.size
+  end
+
 end
diff --git a/test/unit/mail_handler_test.rb b/test/unit/mail_handler_test.rb
index 1e9e7f19e..704e12c7f 100644
--- a/test/unit/mail_handler_test.rb
+++ b/test/unit/mail_handler_test.rb
@@ -133,6 +133,14 @@ class MailHandlerTest < Test::Unit::TestCase
     Role.anonymous.add_permission!(:add_issues)
     assert_equal false, submit_email('ticket_without_from_header.eml')
   end
+
+  def test_add_issue_should_send_email_notification
+    ActionMailer::Base.deliveries.clear
+    # This email contains: 'Project: onlinestore'
+    issue = submit_email('ticket_on_given_project.eml')
+    assert issue.is_a?(Issue)
+    assert_equal 1, ActionMailer::Base.deliveries.size
+  end
   
   def test_add_issue_note
     journal = submit_email('ticket_reply.eml')
@@ -152,6 +160,13 @@ class MailHandlerTest < Test::Unit::TestCase
     assert_match /This is reply/, journal.notes
     assert_equal IssueStatus.find_by_name("Resolved"), issue.status
   end
+
+  def test_add_issue_note_should_send_email_notification
+    ActionMailer::Base.deliveries.clear
+    journal = submit_email('ticket_reply.eml')
+    assert journal.is_a?(Journal)
+    assert_equal 1, ActionMailer::Base.deliveries.size
+  end
   
   def test_reply_to_a_message
     m = submit_email('message_reply.eml')
diff --git a/test/unit/news_test.rb b/test/unit/news_test.rb
index 527715b00..3a908dcc8 100644
--- a/test/unit/news_test.rb
+++ b/test/unit/news_test.rb
@@ -20,9 +20,23 @@ require File.dirname(__FILE__) + '/../test_helper'
 class NewsTest < Test::Unit::TestCase
   fixtures :projects, :users, :roles, :members, :enabled_modules, :news
 
+  def valid_news
+    { :title => 'Test news', :description => 'Lorem ipsum etc', :author => User.find(:first) }
+  end
+
+  
   def setup
   end
   
+  def test_create_should_send_email_notification
+    ActionMailer::Base.deliveries.clear
+    Setting.notified_events << 'news_added'
+    news = Project.find(:first).news.new(valid_news)
+
+    assert news.save
+    assert_equal 1, ActionMailer::Base.deliveries.size
+  end
+  
   def test_should_include_news_for_projects_with_news_enabled
     project = projects(:projects_001)
     assert project.enabled_modules.any?{ |em| em.name == 'news' }
@@ -37,7 +51,7 @@ class NewsTest < Test::Unit::TestCase
     assert ! project.enabled_modules.any?{ |em| em.name == 'news' }
 
     # Add a piece of news to the project
-    news = project.news.create(:title => 'Test news', :description => 'This should not be returned by News.latest')
+    news = project.news.create(valid_news)
 
     # News.latest should not return that new piece of news
     assert News.latest.include?(news) == false
@@ -50,14 +64,14 @@ class NewsTest < Test::Unit::TestCase
   
   def test_should_limit_the_amount_of_returned_news
     # Make sure we have a bunch of news stories
-    10.times { projects(:projects_001).news.create(:title => 'Test news', :description => 'Lorem ipsum etc') }
+    10.times { projects(:projects_001).news.create(valid_news) }
     assert_equal 2, News.latest(users(:users_002), 2).size
     assert_equal 6, News.latest(users(:users_002), 6).size
   end
 
   def test_should_return_5_news_stories_by_default
     # Make sure we have a bunch of news stories
-    10.times { projects(:projects_001).news.create(:title => 'Test news', :description => 'Lorem ipsum etc') }
+    10.times { projects(:projects_001).news.create(valid_news) }
     assert_equal 5, News.latest(users(:users_004)).size
   end
 end