Private issue notes (#1554).

Adds 2 new permissions for viewing/adding private comments to issues.

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

10 files changed, 108 insertions, 27 deletions

diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index 339399329..816a3f521 100644
--- a/app/controllers/issues_controller.rb
+++ b/app/controllers/issues_controller.rb
@@ -99,8 +99,9 @@ class IssuesController < ApplicationController
   end
 
   def show
-    @journals = @issue.journals.find(:all, :include => [:user, :details], :order => "#{Journal.table_name}.created_on ASC")
+    @journals = @issue.journals.includes(:user, :details).reorder("#{Journal.table_name}.id ASC").all
     @journals.each_with_index {|j,i| j.indice = i+1}
+    @journals.reject!(&:private_notes?) unless User.current.allowed_to?(:view_private_notes, @issue.project)
     @journals.reverse! if User.current.wants_comments_in_reverse_order?
 
     @changesets = @issue.changesets.visible.all
@@ -118,7 +119,10 @@ class IssuesController < ApplicationController
       }
       format.api
       format.atom { render :template => 'journals/index', :layout => false, :content_type => 'application/atom+xml' }
-      format.pdf  { send_data(issue_to_pdf(@issue), :type => 'application/pdf', :filename => "#{@project.identifier}-#{@issue.id}.pdf") }
+      format.pdf  {
+        pdf = issue_to_pdf(@issue, :journals => @journals)
+        send_data(pdf, :type => 'application/pdf', :filename => "#{@project.identifier}-#{@issue.id}.pdf")
+      }
     end
   end
 
@@ -173,6 +177,7 @@ class IssuesController < ApplicationController
       @conflict = true
       if params[:last_journal_id]
         @conflict_journals = @issue.journals_after(params[:last_journal_id]).all
+        @conflict_journals.reject!(&:private_notes?) unless User.current.allowed_to?(:view_private_notes, @issue.project)
       end
     end
 
@@ -354,8 +359,7 @@ private
     @time_entry = TimeEntry.new(:issue => @issue, :project => @issue.project)
     @time_entry.attributes = params[:time_entry]
 
-    @notes = params[:notes] || (params[:issue].present? ? params[:issue][:notes] : nil)
-    @issue.init_journal(User.current, @notes)
+    @issue.init_journal(User.current)
 
     issue_attributes = params[:issue]
     if issue_attributes && params[:conflict_resolution]
@@ -364,7 +368,7 @@ private
         issue_attributes = issue_attributes.dup
         issue_attributes.delete(:lock_version)
       when 'add_notes'
-        issue_attributes = {}
+        issue_attributes = issue_attributes.slice(:notes)
       when 'cancel'
         redirect_to issue_path(@issue)
         return false
diff --git a/app/controllers/journals_controller.rb b/app/controllers/journals_controller.rb
index 287d94440..a6e1a9cc9 100644
--- a/app/controllers/journals_controller.rb
+++ b/app/controllers/journals_controller.rb
@@ -57,10 +57,10 @@ class JournalsController < ApplicationController
   end
 
   def new
-    journal = Journal.find(params[:journal_id]) if params[:journal_id]
-    if journal
-      user = journal.user
-      text = journal.notes
+    @journal = Journal.visible.find(params[:journal_id]) if params[:journal_id]
+    if @journal
+      user = @journal.user
+      text = @journal.notes
     else
       user = @issue.author
       text = @issue.description
@@ -69,6 +69,8 @@ class JournalsController < ApplicationController
     text = text.to_s.strip.gsub(%r{<pre>((.|\s)*?)</pre>}m, '[...]')
     @content = "#{ll(Setting.default_language, :text_user_wrote, user)}\n> "
     @content << text.gsub(/(\r?\n|\r\n?)/, "\n> ") + "\n\n"
+  rescue ActiveRecord::RecordNotFound
+    render_404
   end
 
   def edit
@@ -95,7 +97,7 @@ class JournalsController < ApplicationController
   private
 
   def find_journal
-    @journal = Journal.find(params[:id])
+    @journal = Journal.visible.find(params[:id])
     @project = @journal.journalized.project
   rescue ActiveRecord::RecordNotFound
     render_404
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 2f1021b2d..37a19a9c4 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -28,6 +28,14 @@ class Issue < ActiveRecord::Base
   belongs_to :category, :class_name => 'IssueCategory', :foreign_key => 'category_id'
 
   has_many :journals, :as => :journalized, :dependent => :destroy
+  has_many :visible_journals,
+    :class_name => 'Journal',
+    :as => :journalized,
+    :conditions => Proc.new { 
+      ["(#{Journal.table_name}.private_notes = ? OR (#{Project.allowed_to_condition(User.current, :view_private_notes)}))", false]
+    },
+    :readonly => true
+
   has_many :time_entries, :dependent => :delete_all
   has_and_belongs_to_many :changesets, :order => "#{Changeset.table_name}.committed_on ASC, #{Changeset.table_name}.id ASC"
 
@@ -39,7 +47,7 @@ class Issue < ActiveRecord::Base
   acts_as_customizable
   acts_as_watchable
   acts_as_searchable :columns => ['subject', "#{table_name}.description", "#{Journal.table_name}.notes"],
-                     :include => [:project, :journals],
+                     :include => [:project, :visible_journals],
                      # sort by id so that limited eager loading doesn't break with postgresql
                      :order_column => "#{table_name}.id"
   acts_as_event :title => Proc.new {|o| "#{o.tracker.name} ##{o.id} (#{o.status}): #{o.subject}"},
@@ -52,6 +60,7 @@ class Issue < ActiveRecord::Base
   DONE_RATIO_OPTIONS = %w(issue_field issue_status)
 
   attr_reader :current_journal
+  delegate :notes, :notes=, :private_notes, :private_notes=, :to => :current_journal, :allow_nil => true
 
   validates_presence_of :subject, :priority, :project, :tracker, :author, :status
 
@@ -335,6 +344,7 @@ class Issue < ActiveRecord::Base
     'custom_field_values',
     'custom_fields',
     'lock_version',
+    'notes',
     :if => lambda {|issue, user| issue.new_record? || user.allowed_to?(:edit_issues, issue.project) }
 
   safe_attributes 'status_id',
@@ -342,8 +352,15 @@ class Issue < ActiveRecord::Base
     'fixed_version_id',
     'done_ratio',
     'lock_version',
+    'notes',
     :if => lambda {|issue, user| issue.new_statuses_allowed_to(user).any? }
 
+  safe_attributes 'notes',
+    :if => lambda {|issue, user| user.allowed_to?(:add_issue_notes, issue.project)}
+
+  safe_attributes 'private_notes',
+    :if => lambda {|issue, user| !issue.new_record? && user.allowed_to?(:set_notes_private, issue.project)} 
+
   safe_attributes 'watcher_user_ids',
     :if => lambda {|issue, user| issue.new_record? && user.allowed_to?(:add_issue_watchers, issue.project)} 
 
@@ -715,8 +732,8 @@ class Issue < ActiveRecord::Base
     end
   end
 
-  # Returns the mail adresses of users that should be notified
-  def recipients
+  # Returns the users that should be notified
+  def notified_users
     notified = []
     # Author and assignee are always notified unless they have been
     # locked or don't want to be notified
@@ -733,7 +750,12 @@ class Issue < ActiveRecord::Base
     notified.uniq!
     # Remove users that can not view the issue
     notified.reject! {|user| !visible?(user)}
-    notified.collect(&:mail)
+    notified
+  end
+
+  # Returns the email addresses that should be notified
+  def recipients
+    notified_users.collect(&:mail)
   end
 
   # Returns the number of hours spent on this issue
diff --git a/app/models/journal.rb b/app/models/journal.rb
index a6a800e7a..895b8537d 100644
--- a/app/models/journal.rb
+++ b/app/models/journal.rb
@@ -37,10 +37,15 @@ class Journal < ActiveRecord::Base
                                               :conditions => "#{Journal.table_name}.journalized_type = 'Issue' AND" +
                                                              " (#{JournalDetail.table_name}.prop_key = 'status_id' OR #{Journal.table_name}.notes <> '')"}
 
-  scope :visible, lambda {|*args| {
-    :include => {:issue => :project},
-    :conditions => Issue.visible_condition(args.shift || User.current, *args)
-  }}
+  before_create :split_private_notes
+
+  scope :visible, lambda {|*args|
+    user = args.shift || User.current
+
+    includes(:issue => :project).
+      where(Issue.visible_condition(user, *args)).
+      where("(#{Journal.table_name}.private_notes = ? OR (#{Project.allowed_to_condition(user, :view_private_notes, *args)}))", false)
+  }
 
   def save(*args)
     # Do not save an empty journal
@@ -75,6 +80,7 @@ class Journal < ActiveRecord::Base
     s = 'journal'
     s << ' has-notes' unless notes.blank?
     s << ' has-details' unless details.blank?
+    s << ' private-notes' if private_notes?
     s
   end
 
@@ -85,4 +91,33 @@ class Journal < ActiveRecord::Base
   def notify=(arg)
     @notify = arg
   end
+
+  def recipients
+    notified = journalized.notified_users
+    if private_notes?
+      notified = notified.select {|user| user.allowed_to?(:view_private_notes, journalized.project)}
+    end
+    notified.map(&:mail)
+  end
+
+  private
+
+  def split_private_notes
+    if private_notes?
+      if notes.present?
+        if details.any?
+          # Split the journal (notes/changes) so we don't have half-private journals
+          journal = Journal.new(:journalized => journalized, :user => user, :notes => nil, :private_notes => false)
+          journal.details = details
+          journal.save
+          self.details = []
+          self.created_on = journal.created_on
+        end
+      else
+        # Blank notes should not be private
+        self.private_notes = false
+      end
+    end
+    true
+  end
 end
diff --git a/app/models/mail_handler.rb b/app/models/mail_handler.rb
index 8a5925cab..3eacf3c19 100644
--- a/app/models/mail_handler.rb
+++ b/app/models/mail_handler.rb
@@ -181,7 +181,7 @@ class MailHandler < ActionMailer::Base
   end
 
   # Adds a note to an existing issue
-  def receive_issue_reply(issue_id)
+  def receive_issue_reply(issue_id, from_journal=nil)
     issue = Issue.find_by_id(issue_id)
     return unless issue
     # check permission
@@ -196,6 +196,10 @@ class MailHandler < ActionMailer::Base
     @@handler_options[:issue].clear
 
     journal = issue.init_journal(user)
+    if from_journal && from_journal.private_notes?
+      # If the received email was a reply to a private note, make the added note private
+      issue.private_notes = true
+    end
     issue.safe_attributes = issue_attributes_from_keywords(issue)
     issue.safe_attributes = {'custom_field_values' => custom_field_values_from_keywords(issue)}
     journal.notes = cleaned_up_text_body
@@ -211,7 +215,7 @@ class MailHandler < ActionMailer::Base
   def receive_journal_reply(journal_id)
     journal = Journal.find_by_id(journal_id)
     if journal && journal.journalized_type == 'Issue'
-      receive_issue_reply(journal.journalized_id)
+      receive_issue_reply(journal.journalized_id, journal)
     end
   end
 
diff --git a/app/models/mailer.rb b/app/models/mailer.rb
index 65ca9ea08..91b58dda2 100644
--- a/app/models/mailer.rb
+++ b/app/models/mailer.rb
@@ -62,7 +62,7 @@ class Mailer < ActionMailer::Base
     message_id journal
     references issue
     @author = journal.user
-    recipients = issue.recipients
+    recipients = journal.recipients
     # Watchers in cc
     cc = issue.watcher_recipients - recipients
     s = "[#{issue.project.name} - #{issue.tracker.name} ##{issue.id}] "
diff --git a/app/views/issues/_conflict.html.erb b/app/views/issues/_conflict.html.erb
index d2e2a67c4..01d668821 100644
--- a/app/views/issues/_conflict.html.erb
+++ b/app/views/issues/_conflict.html.erb
@@ -18,7 +18,7 @@
 </div>
 <p>
   <label><%= radio_button_tag 'conflict_resolution', 'overwrite' %> <%= l(:text_issue_conflict_resolution_overwrite) %></label><br />
-  <% if @notes.present? %>
+  <% if @issue.notes.present? %>
   <label><%= radio_button_tag 'conflict_resolution', 'add_notes' %> <%= l(:text_issue_conflict_resolution_add_notes) %></label><br />
   <% end %>
   <label><%= radio_button_tag 'conflict_resolution', 'cancel' %> <%= l(:text_issue_conflict_resolution_cancel, :link => link_to_issue(@issue, :subject => false)).html_safe %></label>
diff --git a/app/views/issues/_edit.html.erb b/app/views/issues/_edit.html.erb
index 5b5f53ab6..f2052a27a 100644
--- a/app/views/issues/_edit.html.erb
+++ b/app/views/issues/_edit.html.erb
@@ -27,11 +27,18 @@
     <% end %>
 
     <fieldset><legend><%= l(:field_notes) %></legend>
-    <%= text_area_tag 'notes', @notes, :cols => 60, :rows => 10, :class => 'wiki-edit' %>
-    <%= wikitoolbar_for 'notes' %>
+    <%= f.text_area :notes, :cols => 60, :rows => 10, :class => 'wiki-edit', :no_label => true %>
+    <%= wikitoolbar_for 'issue_notes' %>
+
+    <% if @issue.safe_attribute? 'private_notes' %>
+    <label for="issue_private_notes"><%= f.check_box :private_notes, :no_label => true %> <%= l(:field_private_notes) %></label>
+    <% end %>
+
     <%= call_hook(:view_issues_edit_notes_bottom, { :issue => @issue, :notes => @notes, :form => f }) %>
+    </fieldset>
 
-    <p><%=l(:label_attachment_plural)%><br /><%= render :partial => 'attachments/form', :locals => {:container => @issue} %></p>
+    <fieldset><legend><%= l(:label_attachment_plural) %></legend>
+    <p><%= render :partial => 'attachments/form', :locals => {:container => @issue} %></p>
     </fieldset>
     </div>
 
diff --git a/app/views/issues/show.api.rsb b/app/views/issues/show.api.rsb
index c925587cb..441d1442e 100644
--- a/app/views/issues/show.api.rsb
+++ b/app/views/issues/show.api.rsb
@@ -48,7 +48,7 @@ api.issue do
   end if include_in_api_response?('changesets') && User.current.allowed_to?(:view_changesets, @project)
 
   api.array :journals do
-    @issue.journals.each do |journal|
+    @journals.each do |journal|
       api.journal :id => journal.id do
         api.user(:id => journal.user_id, :name => journal.user.name) unless journal.user.nil?
         api.notes journal.notes
diff --git a/app/views/journals/new.js.erb b/app/views/journals/new.js.erb
index 677bb8d63..78ec5f360 100644
--- a/app/views/journals/new.js.erb
+++ b/app/views/journals/new.js.erb
@@ -1,3 +1,10 @@
-$('#notes').val("<%= raw escape_javascript(@content) %>");
+$('#issue_notes').val("<%= raw escape_javascript(@content) %>");
+<%
+  # when quoting a private journal, check the private checkbox
+  if @journal && @journal.private_notes? 
+%>
+$('#issue_private_notes').attr('checked', true);
+<% end %>
+
 showAndScrollTo("update", "notes");
 $('#notes').scrollTop = $('#notes').scrollHeight - $('#notes').clientHeight;