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

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)
-    @issue.init_journal(User.current, @notes)
 
     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

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]
+    @journal = Journal.visible.find(params[:journal_id]) if params[:journal_id]
-    if journal
+    if @journal
-      user = journal.user
+      user = @journal.user
-      text = journal.notes
+      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

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
+  # Returns the users that should be notified
-  def recipients
+  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

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| {
+  before_create :split_private_notes
-    :include => {:issue => :project},
+
-    :conditions => Issue.visible_condition(args.shift || User.current, *args)
+  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

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
 

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}] "

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>

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' %>
+    <%= f.text_area :notes, :cols => 60, :rows => 10, :class => 'wiki-edit', :no_label => true %>
-    <%= wikitoolbar_for 'notes' %>
+    <%= wikitoolbar_for 'issue_notes' %>
-    <%= call_hook(:view_issues_edit_notes_bottom, { :issue => @issue, :notes => @notes, :form => f }) %>
 
-    <p><%=l(:label_attachment_plural)%><br /><%= render :partial => 'attachments/form', :locals => {:container => @issue} %></p>
+    <% 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>
+
+    <fieldset><legend><%= l(:label_attachment_plural) %></legend>
+    <p><%= render :partial => 'attachments/form', :locals => {:container => @issue} %></p>
     </fieldset>
     </div>
 

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

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;

config/locales/en.yml

@@ -331,6 +331,7 @@ en:
   field_core_fields: Standard fields
   field_timeout: "Timeout (in seconds)"
   field_board_parent: Parent forum
+  field_private_notes: Private notes
 
   setting_app_title: Application title
   setting_app_subtitle: Application subtitle
@@ -416,6 +417,8 @@ en:
   permission_add_issue_notes: Add notes
   permission_edit_issue_notes: Edit notes
   permission_edit_own_issue_notes: Edit own notes
+  permission_view_private_notes: View private notes
+  permission_set_notes_private: Set notes as private
   permission_move_issues: Move issues
   permission_delete_issues: Delete issues
   permission_manage_public_queries: Manage public queries

config/locales/fr.yml

@@ -330,6 +330,7 @@ fr:
   field_core_fields: Champs standards
   field_timeout: "Timeout (en secondes)"
   field_board_parent: Forum parent
+  field_private_notes: Notes privées
 
   setting_app_title: Titre de l'application
   setting_app_subtitle: Sous-titre de l'application
@@ -411,6 +412,8 @@ fr:
   permission_add_issue_notes: Ajouter des notes
   permission_edit_issue_notes: Modifier les notes
   permission_edit_own_issue_notes: Modifier ses propres notes
+  permission_view_private_notes: Voir les notes privées
+  permission_set_notes_private: Rendre les notes privées
   permission_move_issues: Déplacer les demandes
   permission_delete_issues: Supprimer les demandes
   permission_manage_public_queries: Gérer les requêtes publiques

db/migrate/20120930112914_add_journals_private_notes.rb

@@ -0,0 +1,9 @@
+class AddJournalsPrivateNotes < ActiveRecord::Migration
+  def up
+    add_column :journals, :private_notes, :boolean, :default => false, :null => false
+  end
+
+  def down
+    remove_column :journals, :private_notes
+  end
+end

lib/redmine.rb

@@ -78,6 +78,8 @@ Redmine::AccessControl.map do |map|
     map.permission :add_issue_notes, {:issues => [:edit, :update], :journals => [:new], :attachments => :upload}
     map.permission :edit_issue_notes, {:journals => :edit}, :require => :loggedin
     map.permission :edit_own_issue_notes, {:journals => :edit}, :require => :loggedin
+    map.permission :view_private_notes, {}, :read => true, :require => :member
+    map.permission :set_notes_private, {}, :require => :member
     map.permission :move_issues, {:issues => [:bulk_edit, :bulk_update]}, :require => :loggedin
     map.permission :delete_issues, {:issues => :destroy}, :require => :member
     # Queries

lib/redmine/default_data/loader.rb

@@ -53,6 +53,8 @@ module Redmine
                                                       :view_issues,
                                                       :add_issues,
                                                       :edit_issues,
+                                                      :view_private_notes,
+                                                      :set_notes_private,
                                                       :manage_issue_relations,
                                                       :manage_subtasks,
                                                       :add_issue_notes,

lib/redmine/export/pdf.rb

@@ -495,7 +495,7 @@ module Redmine
       end
 
       # Returns a PDF string of a single issue
-      def issue_to_pdf(issue)
+      def issue_to_pdf(issue, assoc={})
         pdf = ITCPDF.new(current_language)
         pdf.SetTitle("#{issue.project} - #{issue.tracker} ##{issue.id}")
         pdf.alias_nb_pages
@@ -642,31 +642,28 @@ module Redmine
           end
         end
 
-        pdf.SetFontStyle('B',9)
+        if assoc[:journals].present?
-        pdf.RDMCell(190,5, l(:label_history), "B")
+          pdf.SetFontStyle('B',9)
-        pdf.Ln
+          pdf.RDMCell(190,5, l(:label_history), "B")
-        indice = 0
-        for journal in issue.journals.find(
-                          :all, :include => [:user, :details],
-                          :order => "#{Journal.table_name}.created_on ASC")
-          indice = indice + 1
-          pdf.SetFontStyle('B',8)
-          pdf.RDMCell(190,5,
-             "#" + indice.to_s +
-             " - " + format_time(journal.created_on) +
-             " - " + journal.user.name)
           pdf.Ln
-          pdf.SetFontStyle('I',8)
+          assoc[:journals].each do |journal|
-          details_to_strings(journal.details, true).each do |string|
+            pdf.SetFontStyle('B',8)
-            pdf.RDMMultiCell(190,5, "- " + string)
+            title = "##{journal.indice} - #{format_time(journal.created_on)} - #{journal.user}"
+            title << " (#{l(:field_private_notes)})" if journal.private_notes?
+            pdf.RDMCell(190,5, title)
+            pdf.Ln
+            pdf.SetFontStyle('I',8)
+            details_to_strings(journal.details, true).each do |string|
+              pdf.RDMMultiCell(190,5, "- " + string)
+            end
+            if journal.notes?
+              pdf.Ln unless journal.details.empty?
+              pdf.SetFontStyle('',8)
+              pdf.RDMwriteHTMLCell(190,5,0,0,
+                    journal.notes.to_s, issue.attachments, "")
+            end
+            pdf.Ln
           end
-          if journal.notes?
-            pdf.Ln unless journal.details.empty?
-            pdf.SetFontStyle('',8)
-            pdf.RDMwriteHTMLCell(190,5,0,0,
-                  journal.notes.to_s, issue.attachments, "")
-          end
-          pdf.Ln
         end
 
         if issue.attachments.any?

public/stylesheets/application.css

@@ -358,6 +358,8 @@ div#issue-changesets div.changeset { border-bottom: 1px solid #ddd; }
 div#issue-changesets p { margin-top: 0; margin-bottom: 1em;}
 
 .journal ul.details img {margin:0 0 -3px 4px;}
+div.journal {overflow:auto;}
+div.journal.private-notes {border-left:2px solid #d22; padding-left:4px; margin-left:-6px;}
 
 div#activity dl, #search-results { margin-left: 2em; }
 div#activity dd, #search-results dd { margin-bottom: 1em; padding-left: 18px; font-size: 0.9em; }

test/fixtures/roles.yml

@@ -24,6 +24,8 @@ roles_001:
     - :view_issue_watchers
     - :add_issue_watchers
     - :set_issues_private
+    - :set_notes_private
+    - :view_private_notes
     - :delete_issue_watchers
     - :manage_public_queries
     - :save_queries

test/functional/activities_controller_test.rb

@@ -149,4 +149,18 @@ class ActivitiesControllerTest < ActionController::TestCase
     assert_template 'common/feed'
     assert_tag :tag => 'title', :content => /Issues/
   end
+
+  def test_index_should_show_private_notes_with_permission_only
+    journal = Journal.create!(:journalized => Issue.find(2), :notes => 'Private notes with searchkeyword', :private_notes => true)
+    @request.session[:user_id] = 2
+
+    get :index
+    assert_response :success
+    assert_include journal, assigns(:events_by_day).values.flatten
+
+    Role.find(1).remove_permission! :view_private_notes
+    get :index
+    assert_response :success
+    assert_not_include journal, assigns(:events_by_day).values.flatten
+  end
 end

test/functional/issues_controller_test.rb

@@ -904,7 +904,7 @@ class IssuesControllerTest < ActionController::TestCase
     assert_tag 'select', :attributes => {:name => 'issue[done_ratio]'}
     assert_tag 'input', :attributes => { :name => 'issue[custom_field_values][2]' }
     assert_no_tag 'input', :attributes => {:name => 'issue[watcher_user_ids][]'}
-    assert_tag 'textarea', :attributes => {:name => 'notes'}
+    assert_tag 'textarea', :attributes => {:name => 'issue[notes]'}
   end
 
   def test_show_should_display_update_form_with_minimal_permissions
@@ -932,7 +932,7 @@ class IssuesControllerTest < ActionController::TestCase
     assert_no_tag 'select', :attributes => {:name => 'issue[done_ratio]'}
     assert_no_tag 'input', :attributes => { :name => 'issue[custom_field_values][2]' }
     assert_no_tag 'input', :attributes => {:name => 'issue[watcher_user_ids][]'}
-    assert_tag 'textarea', :attributes => {:name => 'notes'}
+    assert_tag 'textarea', :attributes => {:name => 'issue[notes]'}
   end
 
   def test_show_should_display_update_form_with_workflow_permissions
@@ -959,7 +959,7 @@ class IssuesControllerTest < ActionController::TestCase
     assert_tag 'select', :attributes => {:name => 'issue[done_ratio]'}
     assert_no_tag 'input', :attributes => { :name => 'issue[custom_field_values][2]' }
     assert_no_tag 'input', :attributes => {:name => 'issue[watcher_user_ids][]'}
-    assert_tag 'textarea', :attributes => {:name => 'notes'}
+    assert_tag 'textarea', :attributes => {:name => 'issue[notes]'}
   end
 
   def test_show_should_not_display_update_form_without_permissions
@@ -1321,6 +1321,20 @@ class IssuesControllerTest < ActionController::TestCase
     assert_tag :td, :content => 'Dave Lopper, John Smith'
   end
 
+  def test_show_should_display_private_notes_with_permission_only
+    journal = Journal.create!(:journalized => Issue.find(2), :notes => 'Privates notes', :private_notes => true, :user_id => 1)
+    @request.session[:user_id] = 2
+
+    get :show, :id => 2
+    assert_response :success
+    assert_include journal, assigns(:journals)
+
+    Role.find(1).remove_permission! :view_private_notes
+    get :show, :id => 2
+    assert_response :success
+    assert_not_include journal, assigns(:journals)
+  end
+
   def test_show_atom
     get :show, :id => 2, :format => 'atom'
     assert_response :success
@@ -2178,14 +2192,14 @@ class IssuesControllerTest < ActionController::TestCase
     context "#update" do
       should "ignore status change" do
         assert_difference 'Journal.count' do
-          put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 3}
+          put :update, :id => 1, :issue => {:status_id => 3, :notes => 'just trying'}
         end
         assert_equal 1, Issue.find(1).status_id
       end
 
       should "ignore attributes changes" do
         assert_difference 'Journal.count' do
-          put :update, :id => 1, :notes => 'just trying', :issue => {:subject => 'changed', :assigned_to_id => 2}
+          put :update, :id => 1, :issue => {:subject => 'changed', :assigned_to_id => 2, :notes => 'just trying'}
         end
         issue = Issue.find(1)
         assert_equal "Can't print recipes", issue.subject
@@ -2205,21 +2219,21 @@ class IssuesControllerTest < ActionController::TestCase
     context "#update" do
       should "accept authorized status" do
         assert_difference 'Journal.count' do
-          put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 3}
+          put :update, :id => 1, :issue => {:status_id => 3, :notes => 'just trying'}
         end
         assert_equal 3, Issue.find(1).status_id
       end
 
       should "ignore unauthorized status" do
         assert_difference 'Journal.count' do
-          put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 2}
+          put :update, :id => 1, :issue => {:status_id => 2, :notes => 'just trying'}
         end
         assert_equal 1, Issue.find(1).status_id
       end
 
       should "accept authorized attributes changes" do
         assert_difference 'Journal.count' do
-          put :update, :id => 1, :notes => 'just trying', :issue => {:assigned_to_id => 2}
+          put :update, :id => 1, :issue => {:assigned_to_id => 2, :notes => 'just trying'}
         end
         issue = Issue.find(1)
         assert_equal 2, issue.assigned_to_id
@@ -2227,7 +2241,7 @@ class IssuesControllerTest < ActionController::TestCase
 
       should "ignore unauthorized attributes changes" do
         assert_difference 'Journal.count' do
-          put :update, :id => 1, :notes => 'just trying', :issue => {:subject => 'changed'}
+          put :update, :id => 1, :issue => {:subject => 'changed', :notes => 'just trying'}
         end
         issue = Issue.find(1)
         assert_equal "Can't print recipes", issue.subject
@@ -2241,21 +2255,21 @@ class IssuesControllerTest < ActionController::TestCase
 
       should "accept authorized status" do
         assert_difference 'Journal.count' do
-          put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 3}
+          put :update, :id => 1, :issue => {:status_id => 3, :notes => 'just trying'}
         end
         assert_equal 3, Issue.find(1).status_id
       end
 
       should "ignore unauthorized status" do
         assert_difference 'Journal.count' do
-          put :update, :id => 1, :notes => 'just trying', :issue => {:status_id => 2}
+          put :update, :id => 1, :issue => {:status_id => 2, :notes => 'just trying'}
         end
         assert_equal 1, Issue.find(1).status_id
       end
 
       should "accept authorized attributes changes" do
         assert_difference 'Journal.count' do
-          put :update, :id => 1, :notes => 'just trying', :issue => {:subject => 'changed', :assigned_to_id => 2}
+          put :update, :id => 1, :issue => {:subject => 'changed', :assigned_to_id => 2, :notes => 'just trying'}
         end
         issue = Issue.find(1)
         assert_equal "changed", issue.subject
@@ -2750,8 +2764,7 @@ class IssuesControllerTest < ActionController::TestCase
     assert_difference('TimeEntry.count', 0) do
       put :update,
            :id => 1,
-           :issue => { :status_id => 2, :assigned_to_id => 3 },
+           :issue => { :status_id => 2, :assigned_to_id => 3, :notes => 'Assigned to dlopper' },
-           :notes => 'Assigned to dlopper',
            :time_entry => { :hours => '', :comments => '', :activity_id => TimeEntryActivity.first }
     end
     assert_redirected_to :action => 'show', :id => '1'
@@ -2772,7 +2785,7 @@ class IssuesControllerTest < ActionController::TestCase
     # anonymous user
     put :update,
          :id => 1,
-         :notes => notes
+         :issue => { :notes => notes }
     assert_redirected_to :action => 'show', :id => '1'
     j = Journal.find(:first, :order => 'id DESC')
     assert_equal notes, j.notes
@@ -2783,13 +2796,47 @@ class IssuesControllerTest < ActionController::TestCase
     assert_mail_body_match notes, mail
   end
 
+  def test_put_update_with_private_note_only
+    notes = 'Private note'
+    @request.session[:user_id] = 2
+
+    assert_difference 'Journal.count' do
+      put :update, :id => 1, :issue => {:notes => notes, :private_notes => '1'}
+      assert_redirected_to :action => 'show', :id => '1'
+    end
+
+    j = Journal.order('id DESC').first
+    assert_equal notes, j.notes
+    assert_equal true, j.private_notes
+  end
+
+  def test_put_update_with_private_note_and_changes
+    notes = 'Private note'
+    @request.session[:user_id] = 2
+
+    assert_difference 'Journal.count', 2 do
+      put :update, :id => 1, :issue => {:subject => 'New subject', :notes => notes, :private_notes => '1'}
+      assert_redirected_to :action => 'show', :id => '1'
+    end
+
+    j = Journal.order('id DESC').first
+    assert_equal notes, j.notes
+    assert_equal true, j.private_notes
+    assert_equal 0, j.details.count
+
+    j = Journal.order('id DESC').offset(1).first
+    assert_nil j.notes
+    assert_equal false, j.private_notes
+    assert_equal 1, j.details.count
+  end
+
   def test_put_update_with_note_and_spent_time
     @request.session[:user_id] = 2
     spent_hours_before = Issue.find(1).spent_hours
     assert_difference('TimeEntry.count') do
       put :update,
            :id => 1,
-           :notes => '2.5 hours added',
+           :issue => { :notes => '2.5 hours added' },
            :time_entry => { :hours => '2.5', :comments => 'test_put_update_with_note_and_spent_time', :activity_id => TimeEntryActivity.first.id }
     end
     assert_redirected_to :action => 'show', :id => '1'
@@ -2816,7 +2863,7 @@ class IssuesControllerTest < ActionController::TestCase
     # anonymous user
     assert_difference 'Attachment.count' do
       put :update, :id => 1,
-        :notes => '',
+        :issue => {:notes => ''},
         :attachments => {'1' => {'file' => uploaded_test_file('testfile.txt', 'text/plain'), 'description' => 'test file'}}
     end
 
@@ -2892,7 +2939,7 @@ class IssuesControllerTest < ActionController::TestCase
       assert_difference 'JournalDetail.count' do
         assert_no_difference 'Attachment.count' do
           put :update, :id => 1,
-            :notes => 'Attachment added',
+            :issue => {:notes => 'Attachment added'},
             :attachments => {'p0' => {'token' => attachment.token}}
           assert_redirected_to '/issues/1'
         end
@@ -2920,7 +2967,7 @@ class IssuesControllerTest < ActionController::TestCase
     # anonymous user
     put :update,
          :id => 1,
-         :notes => '',
+         :issue => {:notes => ''},
          :attachments => {'1' => {'file' => uploaded_test_file('testfile.txt', 'text/plain')}}
     assert_redirected_to :action => 'show', :id => '1'
     assert_equal '1 file(s) could not be saved.', flash[:warning]
@@ -2933,7 +2980,7 @@ class IssuesControllerTest < ActionController::TestCase
 
     put :update,
          :id => 1,
-         :notes => ''
+         :issue => {:notes => ''}
     assert_redirected_to :action => 'show', :id => '1'
 
     issue.reload
@@ -2963,14 +3010,14 @@ class IssuesControllerTest < ActionController::TestCase
     assert_no_difference('Journal.count') do
       put :update,
            :id => 1,
-           :notes => notes,
+           :issue => {:notes => notes},
            :time_entry => {"comments"=>"", "activity_id"=>"", "hours"=>"2z"}
     end
     assert_response :success
     assert_template 'edit'
 
     assert_error_tag :descendant => {:content => /Activity can&#x27;t be blank/}
-    assert_tag :textarea, :attributes => { :name => 'notes' }, :content => "\n"+notes
+    assert_tag :textarea, :attributes => { :name => 'issue[notes]' }, :content => "\n"+notes
     assert_tag :input, :attributes => { :name => 'time_entry[hours]', :value => "2z" }
   end
 
@@ -2981,7 +3028,7 @@ class IssuesControllerTest < ActionController::TestCase
     assert_no_difference('Journal.count') do
       put :update,
            :id => 1,
-           :notes => notes,
+           :issue => {:notes => notes},
            :time_entry => {"comments"=>"this is my comment", "activity_id"=>"", "hours"=>""}
     end
     assert_response :success
@@ -2989,7 +3036,7 @@ class IssuesControllerTest < ActionController::TestCase
 
     assert_error_tag :descendant => {:content => /Activity can&#x27;t be blank/}
     assert_error_tag :descendant => {:content => /Hours can&#x27;t be blank/}
-    assert_tag :textarea, :attributes => { :name => 'notes' }, :content => "\n"+notes
+    assert_tag :textarea, :attributes => { :name => 'issue[notes]' }, :content => "\n"+notes
     assert_tag :input, :attributes => { :name => 'time_entry[comments]', :value => "this is my comment" }
   end
 

test/functional/issues_controller_transaction_test.rb

@@ -62,9 +62,9 @@ class IssuesControllerTransactionTest < ActionController::TestCase
               :id => issue.id,
               :issue => {
                 :fixed_version_id => 4,
+                :notes => 'My notes',
                 :lock_version => (issue.lock_version - 1)
               },
-              :notes => 'My notes',
               :time_entry => { :hours => '2.5', :comments => '', :activity_id => TimeEntryActivity.first.id }
       end
     end
@@ -93,9 +93,9 @@ class IssuesControllerTransactionTest < ActionController::TestCase
                 :id => issue.id,
                 :issue => {
                   :fixed_version_id => 4,
+                  :notes => 'My notes',
                   :lock_version => (issue.lock_version - 1)
                 },
-                :notes => 'My notes',
                 :attachments => {'1' => {'file' => uploaded_test_file('testfile.txt', 'text/plain')}},
                 :time_entry => { :hours => '2.5', :comments => '', :activity_id => TimeEntryActivity.first.id }
         end
@@ -116,15 +116,14 @@ class IssuesControllerTransactionTest < ActionController::TestCase
     put :update, :id => issue.id,
           :issue => {
             :fixed_version_id => 4,
+            :notes => '',
             :lock_version => (issue.lock_version - 1)
-          },
+          }
-          :notes => ''
 
     assert_tag 'div', :attributes => {:class => 'conflict'}
     assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'overwrite'}
     assert_no_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'add_notes'}
     assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'cancel'}
-    
   end
 
   def test_update_stale_issue_should_show_conflicting_journals
@@ -133,9 +132,9 @@ class IssuesControllerTransactionTest < ActionController::TestCase
     put :update, :id => 1,
           :issue => {
             :fixed_version_id => 4,
+            :notes => '',
             :lock_version => 2
           },
-          :notes => '',
           :last_journal_id => 1
 
     assert_not_nil assigns(:conflict_journals)
@@ -151,9 +150,9 @@ class IssuesControllerTransactionTest < ActionController::TestCase
     put :update, :id => 1,
           :issue => {
             :fixed_version_id => 4,
+            :notes => '',
             :lock_version => 2
           },
-          :notes => '',
           :last_journal_id => ''
 
     assert_not_nil assigns(:conflict_journals)
@@ -164,6 +163,18 @@ class IssuesControllerTransactionTest < ActionController::TestCase
       :descendant => {:content => /Journal notes/}
   end
 
+  def test_update_stale_issue_should_show_private_journals_with_permission_only
+    journal = Journal.create!(:journalized => Issue.find(1), :notes => 'Privates notes', :private_notes => true, :user_id => 1)
+
+    @request.session[:user_id] = 2
+    put :update, :id => 1, :issue => {:fixed_version_id => 4, :lock_version => 2}, :last_journal_id => ''
+    assert_include journal, assigns(:conflict_journals)
+
+    Role.find(1).remove_permission! :view_private_notes
+    put :update, :id => 1, :issue => {:fixed_version_id => 4, :lock_version => 2}, :last_journal_id => ''
+    assert_not_include journal, assigns(:conflict_journals)
+  end
+
   def test_update_stale_issue_with_overwrite_conflict_resolution_should_update
     @request.session[:user_id] = 2
 
@@ -171,9 +182,9 @@ class IssuesControllerTransactionTest < ActionController::TestCase
       put :update, :id => 1,
             :issue => {
               :fixed_version_id => 4,
+              :notes => 'overwrite_conflict_resolution',
               :lock_version => 2
             },
-            :notes => 'overwrite_conflict_resolution',
             :conflict_resolution => 'overwrite'
     end
 
@@ -192,9 +203,9 @@ class IssuesControllerTransactionTest < ActionController::TestCase
       put :update, :id => 1,
             :issue => {
               :fixed_version_id => 4,
+              :notes => 'add_notes_conflict_resolution',
               :lock_version => 2
             },
-            :notes => 'add_notes_conflict_resolution',
             :conflict_resolution => 'add_notes'
     end
 
@@ -213,9 +224,9 @@ class IssuesControllerTransactionTest < ActionController::TestCase
       put :update, :id => 1,
             :issue => {
               :fixed_version_id => 4,
+              :notes => 'add_notes_conflict_resolution',
               :lock_version => 2
             },
-            :notes => 'add_notes_conflict_resolution',
             :conflict_resolution => 'cancel'
     end
 

test/functional/journals_controller_test.rb

@@ -39,6 +39,20 @@ class JournalsControllerTest < ActionController::TestCase
     assert_equal 'application/atom+xml', @response.content_type
   end
 
+  def test_index_should_return_privates_notes_with_permission_only
+    journal = Journal.create!(:journalized => Issue.find(2), :notes => 'Privates notes', :private_notes => true, :user_id => 1)
+    @request.session[:user_id] = 2
+
+    get :index, :project_id => 1
+    assert_response :success
+    assert_include journal, assigns(:journals)
+
+    Role.find(1).remove_permission! :view_private_notes
+    get :index, :project_id => 1
+    assert_response :success
+    assert_not_include journal, assigns(:journals)
+  end
+
   def test_diff
     get :diff, :id => 3, :detail_id => 4
     assert_response :success
@@ -76,6 +90,21 @@ class JournalsControllerTest < ActionController::TestCase
     assert_include '> A comment with a private version', response.body
   end
 
+  def test_reply_to_private_note_should_fail_without_permission
+    journal = Journal.create!(:journalized => Issue.find(2), :notes => 'Privates notes', :private_notes => true)
+    @request.session[:user_id] = 2
+
+    xhr :get, :new, :id => 2, :journal_id => journal.id
+    assert_response :success
+    assert_template 'new'
+    assert_equal 'text/javascript', response.content_type
+    assert_include '> Privates notes', response.body
+
+    Role.find(1).remove_permission! :view_private_notes
+    xhr :get, :new, :id => 2, :journal_id => journal.id
+    assert_response 404
+  end
+
   def test_edit_xhr
     @request.session[:user_id] = 1
     xhr :get, :edit, :id => 2
@@ -85,6 +114,22 @@ class JournalsControllerTest < ActionController::TestCase
     assert_include 'textarea', response.body
   end
 
+  def test_edit_private_note_should_fail_without_permission
+    journal = Journal.create!(:journalized => Issue.find(2), :notes => 'Privates notes', :private_notes => true)
+    @request.session[:user_id] = 2
+    Role.find(1).add_permission! :edit_issue_notes
+
+    xhr :get, :edit, :id => journal.id
+    assert_response :success
+    assert_template 'edit'
+    assert_equal 'text/javascript', response.content_type
+    assert_include 'textarea', response.body
+
+    Role.find(1).remove_permission! :view_private_notes
+    xhr :get, :edit, :id => journal.id
+    assert_response 404
+  end
+
   def test_update_xhr
     @request.session[:user_id] = 1
     xhr :post, :edit, :id => 2, :notes => 'Updated notes'

test/functional/search_controller_test.rb

@@ -58,6 +58,39 @@ class SearchControllerTest < ActionController::TestCase
                     :child => { :tag => 'a',  :content => /Closed/ }
   end
 
+  def test_search_issues_should_search_notes
+    Journal.create!(:journalized => Issue.find(2), :notes => 'Issue notes with searchkeyword')
+
+    get :index, :q => 'searchkeyword', :issues => 1
+    assert_response :success
+    assert_include Issue.find(2), assigns(:results)
+  end
+
+  def test_search_issues_with_multiple_matches_in_journals_should_return_issue_once
+    Journal.create!(:journalized => Issue.find(2), :notes => 'Issue notes with searchkeyword')
+    Journal.create!(:journalized => Issue.find(2), :notes => 'Issue notes with searchkeyword')
+
+    get :index, :q => 'searchkeyword', :issues => 1
+    assert_response :success
+    assert_include Issue.find(2), assigns(:results)
+    assert_equal 1, assigns(:results).size
+  end
+
+  def test_search_issues_should_search_private_notes_with_permission_only
+    Journal.create!(:journalized => Issue.find(2), :notes => 'Private notes with searchkeyword', :private_notes => true)
+    @request.session[:user_id] = 2
+
+    Role.find(1).add_permission! :view_private_notes
+    get :index, :q => 'searchkeyword', :issues => 1
+    assert_response :success
+    assert_include Issue.find(2), assigns(:results)
+
+    Role.find(1).remove_permission! :view_private_notes
+    get :index, :q => 'searchkeyword', :issues => 1
+    assert_response :success
+    assert_not_include Issue.find(2), assigns(:results)
+  end
+
   def test_search_all_projects_with_scope_param
     get :index, :q => 'issue', :scope => 'all'
     assert_response :success

test/object_helpers.rb

@@ -90,6 +90,15 @@ module ObjectHelpers
     issue.reload
   end
 
+  def Journal.generate!(attributes={})
+    journal = Journal.new(attributes)
+    journal.user ||= User.first
+    journal.journalized ||= Issue.first
+    yield journal if block_given?
+    journal.save!
+    journal
+  end
+
   def Version.generate!(attributes={})
     @generated_version_name ||= 'Version 0'
     @generated_version_name.succ!

test/unit/journal_test.rb

@@ -47,6 +47,71 @@ class JournalTest < ActiveSupport::TestCase
     assert_equal 1, ActionMailer::Base.deliveries.size
   end
 
+  def test_should_not_save_journal_with_blank_notes_and_no_details
+    journal = Journal.new(:journalized => Issue.first, :user => User.first)
+
+    assert_no_difference 'Journal.count' do
+      assert_equal false, journal.save
+    end
+  end
+
+  def test_create_should_not_split_non_private_notes
+    assert_difference 'Journal.count' do
+      assert_no_difference 'JournalDetail.count' do
+        journal = Journal.generate!(:notes => 'Notes')
+      end
+    end
+
+    assert_difference 'Journal.count' do
+      assert_difference 'JournalDetail.count' do
+        journal = Journal.generate!(:notes => 'Notes', :details => [JournalDetail.new])
+      end
+    end
+
+    assert_difference 'Journal.count' do
+      assert_difference 'JournalDetail.count' do
+        journal = Journal.generate!(:notes => '', :details => [JournalDetail.new])
+      end
+    end
+  end
+
+  def test_create_should_split_private_notes
+    assert_difference 'Journal.count' do
+      assert_no_difference 'JournalDetail.count' do
+        journal = Journal.generate!(:notes => 'Notes', :private_notes => true)
+        journal.reload
+        assert_equal true, journal.private_notes
+        assert_equal 'Notes', journal.notes
+      end
+    end
+
+    assert_difference 'Journal.count', 2 do
+      assert_difference 'JournalDetail.count' do
+        journal = Journal.generate!(:notes => 'Notes', :private_notes => true, :details => [JournalDetail.new])
+        journal.reload
+        assert_equal true, journal.private_notes
+        assert_equal 'Notes', journal.notes
+        assert_equal 0, journal.details.size
+
+        journal_with_changes = Journal.order('id DESC').offset(1).first
+        assert_equal false, journal_with_changes.private_notes
+        assert_nil journal_with_changes.notes
+        assert_equal 1, journal_with_changes.details.size
+        assert_equal journal.created_on, journal_with_changes.created_on
+      end
+    end
+
+    assert_difference 'Journal.count' do
+      assert_difference 'JournalDetail.count' do
+        journal = Journal.generate!(:notes => '', :private_notes => true, :details => [JournalDetail.new])
+        journal.reload
+        assert_equal false, journal.private_notes
+        assert_equal '', journal.notes
+        assert_equal 1, journal.details.size
+      end
+    end
+  end
+
   def test_visible_scope_for_anonymous
     # Anonymous user should see issues of public projects only
     journals = Journal.visible(User.anonymous).all

test/unit/mail_handler_test.rb

@@ -435,6 +435,7 @@ class MailHandlerTest < ActiveSupport::TestCase
     assert_equal User.find_by_login('jsmith'), journal.user
     assert_equal Issue.find(2), journal.journalized
     assert_match /This is reply/, journal.notes
+    assert_equal false, journal.private_notes
     assert_equal 'Feature request', journal.issue.tracker.name
   end
 
@@ -496,6 +497,20 @@ class MailHandlerTest < ActiveSupport::TestCase
     assert_equal 'Normal', journal.issue.priority.name
   end
 
+  def test_replying_to_a_private_note_should_add_reply_as_private
+    private_journal = Journal.create!(:notes => 'Private notes', :journalized => Issue.find(1), :private_notes => true, :user_id => 2)
+
+    assert_difference 'Journal.count' do
+      journal = submit_email('ticket_reply.eml') do |email|
+        email.sub! %r{^In-Reply-To:.*$}, "In-Reply-To: <redmine.journal-#{private_journal.id}.20060719210421@osiris>"
+      end
+
+      assert_kind_of Journal, journal
+      assert_match /This is reply/, journal.notes
+      assert_equal true, journal.private_notes
+    end
+  end
+
   def test_reply_to_a_message
     m = submit_email('message_reply.eml')
     assert m.is_a?(Message)

test/unit/mailer_test.rb

@@ -336,6 +336,20 @@ class MailerTest < ActiveSupport::TestCase
     end
   end
 
+  def test_issue_edit_should_send_private_notes_to_users_with_permission_only
+    journal = Journal.find(1)
+    journal.private_notes = true
+    journal.save!
+
+    Role.find(2).add_permission! :view_private_notes
+    Mailer.issue_edit(journal).deliver
+    assert_equal %w(dlopper@somenet.foo jsmith@somenet.foo), ActionMailer::Base.deliveries.last.bcc.sort
+
+    Role.find(2).remove_permission! :view_private_notes
+    Mailer.issue_edit(journal).deliver
+    assert_equal %w(jsmith@somenet.foo), ActionMailer::Base.deliveries.last.bcc.sort
+  end
+
   def test_document_added
     document = Document.find(1)
     valid_languages.each do |lang|