From 0178b5a2fe07e1160348b99ac56c19ebf154ca1b Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Wed, 3 Oct 2012 21:36:19 +0000 Subject: [PATCH] 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 | 14 ++- app/controllers/journals_controller.rb | 12 ++- app/models/issue.rb | 30 +++++- app/models/journal.rb | 43 ++++++++- app/models/mail_handler.rb | 8 +- app/models/mailer.rb | 2 +- app/views/issues/_conflict.html.erb | 2 +- app/views/issues/_edit.html.erb | 15 ++- app/views/issues/show.api.rsb | 2 +- app/views/journals/new.js.erb | 9 +- config/locales/en.yml | 3 + config/locales/fr.yml | 3 + ...120930112914_add_journals_private_notes.rb | 9 ++ lib/redmine.rb | 2 + lib/redmine/default_data/loader.rb | 2 + lib/redmine/export/pdf.rb | 45 ++++----- public/stylesheets/application.css | 2 + test/fixtures/roles.yml | 2 + test/functional/activities_controller_test.rb | 14 +++ test/functional/issues_controller_test.rb | 95 ++++++++++++++----- .../issues_controller_transaction_test.rb | 31 ++++-- test/functional/journals_controller_test.rb | 45 +++++++++ test/functional/search_controller_test.rb | 33 +++++++ test/object_helpers.rb | 9 ++ test/unit/journal_test.rb | 65 +++++++++++++ test/unit/mail_handler_test.rb | 15 +++ test/unit/mailer_test.rb | 14 +++ 27 files changed, 440 insertions(+), 86 deletions(-) create mode 100644 db/migrate/20120930112914_add_journals_private_notes.rb 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{
((.|\s)*?)
}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 @@


- <% if @notes.present? %> + <% if @issue.notes.present? %>
<% end %> 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 %>

<%= l(:field_notes) %> - <%= text_area_tag 'notes', @notes, :cols => 60, :rows => 10, :class => 'wiki-edit' %> - <%= wikitoolbar_for 'notes' %> - <%= call_hook(:view_issues_edit_notes_bottom, { :issue => @issue, :notes => @notes, :form => f }) %> + <%= f.text_area :notes, :cols => 60, :rows => 10, :class => 'wiki-edit', :no_label => true %> + <%= wikitoolbar_for 'issue_notes' %> -

<%=l(:label_attachment_plural)%>
<%= render :partial => 'attachments/form', :locals => {:container => @issue} %>

+ <% if @issue.safe_attribute? 'private_notes' %> + + <% end %> + + <%= call_hook(:view_issues_edit_notes_bottom, { :issue => @issue, :notes => @notes, :form => f }) %> +
+ +
<%= l(:label_attachment_plural) %> +

<%= render :partial => 'attachments/form', :locals => {:container => @issue} %>

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; diff --git a/config/locales/en.yml b/config/locales/en.yml index b72dfea66..9b1da6153 100644 --- a/config/locales/en.yml +++ b/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 diff --git a/config/locales/fr.yml b/config/locales/fr.yml index d2ab50ffc..ba9311bc5 100644 --- a/config/locales/fr.yml +++ b/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 diff --git a/db/migrate/20120930112914_add_journals_private_notes.rb b/db/migrate/20120930112914_add_journals_private_notes.rb new file mode 100644 index 000000000..41eb0e9ef --- /dev/null +++ b/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 diff --git a/lib/redmine.rb b/lib/redmine.rb index 0459dcc50..e120aa802 100644 --- a/lib/redmine.rb +++ b/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 diff --git a/lib/redmine/default_data/loader.rb b/lib/redmine/default_data/loader.rb index eabc35161..a91f6e22c 100644 --- a/lib/redmine/default_data/loader.rb +++ b/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, diff --git a/lib/redmine/export/pdf.rb b/lib/redmine/export/pdf.rb index e821c4e8a..e554144e2 100644 --- a/lib/redmine/export/pdf.rb +++ b/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) - pdf.RDMCell(190,5, l(:label_history), "B") - pdf.Ln - 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) + if assoc[:journals].present? + pdf.SetFontStyle('B',9) + pdf.RDMCell(190,5, l(:label_history), "B") pdf.Ln - pdf.SetFontStyle('I',8) - details_to_strings(journal.details, true).each do |string| - pdf.RDMMultiCell(190,5, "- " + string) + assoc[:journals].each do |journal| + pdf.SetFontStyle('B',8) + 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? diff --git a/public/stylesheets/application.css b/public/stylesheets/application.css index fff82cedf..c6b68df6d 100644 --- a/public/stylesheets/application.css +++ b/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; } diff --git a/test/fixtures/roles.yml b/test/fixtures/roles.yml index 78b35c16e..1cacf0100 100644 --- a/test/fixtures/roles.yml +++ b/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 diff --git a/test/functional/activities_controller_test.rb b/test/functional/activities_controller_test.rb index 13f44a108..234ea7c57 100644 --- a/test/functional/activities_controller_test.rb +++ b/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 diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb index e5079b5a6..7672a3e75 100644 --- a/test/functional/issues_controller_test.rb +++ b/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 }, - :notes => 'Assigned to dlopper', + :issue => { :status_id => 2, :assigned_to_id => 3, :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'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't be blank/} assert_error_tag :descendant => {:content => /Hours can'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 diff --git a/test/functional/issues_controller_transaction_test.rb b/test/functional/issues_controller_transaction_test.rb index 84f9adc91..7e1fa0d00 100644 --- a/test/functional/issues_controller_transaction_test.rb +++ b/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 diff --git a/test/functional/journals_controller_test.rb b/test/functional/journals_controller_test.rb index 652edf68d..5ba4725eb 100644 --- a/test/functional/journals_controller_test.rb +++ b/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' diff --git a/test/functional/search_controller_test.rb b/test/functional/search_controller_test.rb index 9491750bb..9bcb72dbd 100644 --- a/test/functional/search_controller_test.rb +++ b/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 diff --git a/test/object_helpers.rb b/test/object_helpers.rb index 0100a8bed..e0ed4ee98 100644 --- a/test/object_helpers.rb +++ b/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! diff --git a/test/unit/journal_test.rb b/test/unit/journal_test.rb index 2a7d5f46a..a2669cec1 100644 --- a/test/unit/journal_test.rb +++ b/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 diff --git a/test/unit/mail_handler_test.rb b/test/unit/mail_handler_test.rb index 49b5d896f..a1e5b8178 100644 --- a/test/unit/mail_handler_test.rb +++ b/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: " + 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) diff --git a/test/unit/mailer_test.rb b/test/unit/mailer_test.rb index efde01f5c..8d65cc5b7 100644 --- a/test/unit/mailer_test.rb +++ b/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|