Make sure users don't get notified for thing they can not view (#3589).

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

app/models/document.rb

@@ -29,6 +29,10 @@ class Document < ActiveRecord::Base
   validates_presence_of :project, :title, :category
   validates_length_of :title, :maximum => 60
   
+  def visible?(user=User.current)
+    !user.nil? && user.allowed_to?(:view_documents, project)
+  end
+  
   def after_initialize
     if new_record?
       self.category ||= DocumentCategory.default
@@ -42,4 +46,11 @@ class Document < ActiveRecord::Base
     end
     @updated_on
   end
+  
+  # Returns the mail adresses of users that should be notified
+  def recipients
+    notified = project.notified_users
+    notified.reject! {|user| !visible?(user)}
+    notified.collect(&:mail)
+  end
 end

app/models/mailer.rb

@@ -94,7 +94,7 @@ class Mailer < ActionMailer::Base
   #   Mailer.deliver_document_added(document) => sends an email to the document's project recipients
   def document_added(document)
     redmine_headers 'Project' => document.project.identifier
-    recipients document.project.recipients
+    recipients document.recipients
     subject "[#{document.project.name}] #{l(:label_document_new)}: #{document.title}"
     body :document => document,
          :document_url => url_for(:controller => 'documents', :action => 'show', :id => document)
@@ -114,15 +114,17 @@ class Mailer < ActionMailer::Base
     when 'Project'
       added_to_url = url_for(:controller => 'projects', :action => 'list_files', :id => container)
       added_to = "#{l(:label_project)}: #{container}"
+      recipients container.project.notified_users.select {|user| user.allowed_to?(:view_files, container.project)}
     when 'Version'
       added_to_url = url_for(:controller => 'projects', :action => 'list_files', :id => container.project_id)
       added_to = "#{l(:label_version)}: #{container.name}"
+      recipients container.project.notified_users.select {|user| user.allowed_to?(:view_files, container.project)}
     when 'Document'
       added_to_url = url_for(:controller => 'documents', :action => 'show', :id => container.id)
       added_to = "#{l(:label_document)}: #{container.title}"
+      recipients container.recipients
     end
     redmine_headers 'Project' => container.project.identifier
-    recipients container.project.recipients
     subject "[#{container.project.name}] #{l(:label_attachment_new)}"
     body :attachments => attachments,
          :added_to => added_to,
@@ -138,24 +140,25 @@ class Mailer < ActionMailer::Base
   def news_added(news)
     redmine_headers 'Project' => news.project.identifier
     message_id news
-    recipients news.project.recipients
+    recipients news.recipients
     subject "[#{news.project.name}] #{l(:label_news)}: #{news.title}"
     body :news => news,
          :news_url => url_for(:controller => 'news', :action => 'show', :id => news)
     render_multipart('news_added', body)
   end
 
-  # Builds a tmail object used to email the specified recipients of the specified message that was posted. 
+  # Builds a tmail object used to email the recipients of the specified message that was posted. 
   #
   # Example:
-  #   message_posted(message, recipients) => tmail object
-  #   Mailer.deliver_message_posted(message, recipients) => sends an email to the recipients
-  def message_posted(message, recipients)
+  #   message_posted(message) => tmail object
+  #   Mailer.deliver_message_posted(message) => sends an email to the recipients
+  def message_posted(message)
     redmine_headers 'Project' => message.project.identifier,
                     'Topic-Id' => (message.parent_id || message.id)
     message_id message
     references message.parent unless message.parent.nil?
-    recipients(recipients)
+    recipients(message.recipients)
+    cc((message.root.watcher_recipients + message.board.watcher_recipients).uniq - @recipients)
     subject "[#{message.board.project.name} - #{message.board.name} - msg#{message.root.id}] #{message.subject}"
     body :message => message,
          :message_url => url_for(:controller => 'messages', :action => 'show', :board_id => message.board_id, :id => message.root)
@@ -171,7 +174,7 @@ class Mailer < ActionMailer::Base
     redmine_headers 'Project' => wiki_content.project.identifier,
                     'Wiki-Page-Id' => wiki_content.page.id
     message_id wiki_content
-    recipients wiki_content.project.recipients
+    recipients wiki_content.recipients
     cc(wiki_content.page.wiki.watcher_recipients - recipients)
     subject "[#{wiki_content.project.name}] #{l(:mail_subject_wiki_content_added, :page => wiki_content.page.pretty_title)}"
     body :wiki_content => wiki_content,
@@ -188,7 +191,7 @@ class Mailer < ActionMailer::Base
     redmine_headers 'Project' => wiki_content.project.identifier,
                     'Wiki-Page-Id' => wiki_content.page.id
     message_id wiki_content
-    recipients wiki_content.project.recipients
+    recipients wiki_content.recipients
     cc(wiki_content.page.wiki.watcher_recipients + wiki_content.page.watcher_recipients - recipients)
     subject "[#{wiki_content.project.name}] #{l(:mail_subject_wiki_content_updated, :page => wiki_content.page.pretty_title)}"
     body :wiki_content => wiki_content,

app/models/message.rb

@@ -90,6 +90,13 @@ class Message < ActiveRecord::Base
     usr && usr.logged? && (usr.allowed_to?(:delete_messages, project) || (self.author == usr && usr.allowed_to?(:delete_own_messages, project)))
   end
   
+  # Returns the mail adresses of users that should be notified
+  def recipients
+    notified = project.notified_users
+    notified.reject! {|user| !visible?(user)}
+    notified.collect(&:mail)
+  end
+  
   private
   
   def add_author_as_watcher

app/models/message_observer.rb

@@ -17,14 +17,6 @@
 
 class MessageObserver < ActiveRecord::Observer
   def after_create(message)
-    recipients = []
-    # send notification to the topic watchers
-    recipients += message.root.watcher_recipients
-    # send notification to the board watchers
-    recipients += message.board.watcher_recipients
-    # send notification to project members who want to be notified
-    recipients += message.board.project.recipients
-    recipients = recipients.compact.uniq
-    Mailer.deliver_message_posted(message, recipients) if !recipients.empty? && Setting.notified_events.include?('message_posted')
+    Mailer.deliver_message_posted(message) if Setting.notified_events.include?('message_posted')
   end
 end

app/models/news.rb

@@ -29,6 +29,17 @@ class News < ActiveRecord::Base
   acts_as_activity_provider :find_options => {:include => [:project, :author]},
                             :author_key => :author_id
   
+  def visible?(user=User.current)
+    !user.nil? && user.allowed_to?(:view_news, project)
+  end
+  
+  # Returns the mail adresses of users that should be notified
+  def recipients
+    notified = project.notified_users
+    notified.reject! {|user| !visible?(user)}
+    notified.collect(&:mail)
+  end
+  
   # returns latest news for projects visible by user
   def self.latest(user = User.current, count = 5)
     find(:all, :limit => count, :conditions => Project.allowed_to_condition(user, :view_news), :include => [ :author, :project ], :order => "#{News.table_name}.created_on DESC")	

app/models/wiki_content.rb

@@ -25,11 +25,22 @@ class WikiContent < ActiveRecord::Base
   validates_length_of :comments, :maximum => 255, :allow_nil => true
   
   acts_as_versioned
+  
+  def visible?(user=User.current)
+    page.visible?(user)
+  end
     
   def project
     page.project
   end
   
+  # Returns the mail adresses of users that should be notified
+  def recipients
+    notified = project.notified_users
+    notified.reject! {|user| !visible?(user)}
+    notified.collect(&:mail)
+  end
+  
   class Version
     belongs_to :page, :class_name => '::WikiPage', :foreign_key => 'page_id'
     belongs_to :author, :class_name => '::User', :foreign_key => 'author_id'

test/unit/mailer_test.rb

@@ -147,7 +147,7 @@ class MailerTest < ActiveSupport::TestCase
   def test_message_posted_message_id
     ActionMailer::Base.deliveries.clear
     message = Message.find(1)
-    Mailer.deliver_message_posted(message, message.author.mail)
+    Mailer.deliver_message_posted(message)
     mail = ActionMailer::Base.deliveries.last
     assert_not_nil mail
     assert_equal Mailer.message_id_for(message), mail.message_id
@@ -157,13 +157,47 @@ class MailerTest < ActiveSupport::TestCase
   def test_reply_posted_message_id
     ActionMailer::Base.deliveries.clear
     message = Message.find(3)
-    Mailer.deliver_message_posted(message, message.author.mail)
+    Mailer.deliver_message_posted(message)
     mail = ActionMailer::Base.deliveries.last
     assert_not_nil mail
     assert_equal Mailer.message_id_for(message), mail.message_id
     assert_equal Mailer.message_id_for(message.parent), mail.references.first.to_s
   end
   
+  context("#issue_add") do
+    setup do
+      ActionMailer::Base.deliveries.clear
+      Setting.bcc_recipients = '1'
+      @issue = Issue.find(1) 
+    end
+    
+    should "notify project members" do
+      assert Mailer.deliver_issue_add(@issue)
+      assert last_email.bcc.include?('dlopper@somenet.foo')
+    end
+    
+    should "not notify project members that are not allow to view the issue" do
+      Role.find(2).remove_permission!(:view_issues)
+      assert Mailer.deliver_issue_add(@issue)
+      assert !last_email.bcc.include?('dlopper@somenet.foo')
+    end
+    
+    should "notify issue watchers" do
+      user = User.find(9)
+      Watcher.create!(:watchable => @issue, :user => user)
+      assert Mailer.deliver_issue_add(@issue)
+      assert last_email.bcc.include?(user.mail)
+    end
+    
+    should "not notify watchers not allowed to view the issue" do
+      user = User.find(9)
+      Watcher.create!(:watchable => @issue, :user => user)
+      Role.non_member.remove_permission!(:view_issues)
+      assert Mailer.deliver_issue_add(@issue)
+      assert !last_email.bcc.include?(user.mail)
+    end
+  end
+  
   # test mailer methods for each language
   def test_issue_add
     issue = Issue.find(1)
@@ -211,7 +245,7 @@ class MailerTest < ActiveSupport::TestCase
     recipients = recipients.compact.uniq
     valid_languages.each do |lang|
       Setting.default_language = lang.to_s
-      assert Mailer.deliver_message_posted(message, recipients)
+      assert Mailer.deliver_message_posted(message)
     end
   end
   
@@ -256,4 +290,10 @@ class MailerTest < ActiveSupport::TestCase
     assert mail.bcc.include?('dlopper@somenet.foo')
     assert mail.body.include?('Bug #3: Error 281 when updating a recipe')
   end
+  
+  def last_email
+    mail = ActionMailer::Base.deliveries.last
+    assert_not_nil mail
+    mail
+  end
 end