Show long text custom field changes as a diff (#15236).

git-svn-id: http://svn.redmine.org/redmine/trunk@13954 e93f8b46-1217-0410-a6f0-8f06a7374b81

5 files changed, 65 insertions, 7 deletions

diff --git a/app/controllers/journals_controller.rb b/app/controllers/journals_controller.rb
index fd75d752a..bae6ca2bc 100644
--- a/app/controllers/journals_controller.rb
+++ b/app/controllers/journals_controller.rb
@@ -49,9 +49,17 @@ class JournalsController < ApplicationController
     if params[:detail_id].present?
       @detail = @journal.details.find_by_id(params[:detail_id])
     else
-      @detail = @journal.details.detect {|d| d.prop_key == 'description'}
+      @detail = @journal.details.detect {|d| d.property == 'attr' && d.prop_key == 'description'}
+    end
+    unless @issue && @detail
+      render_404
+      return false
+    end
+    if @detail.property == 'cf'
+      unless @detail.custom_field && @detail.custom_field.visible_by?(@issue.project, User.current)
+        raise ::Unauthorized
+      end
     end
-    (render_404; return false) unless @issue && @detail
     @diff = Redmine::Helpers::Diff.new(@detail.value, @detail.old_value)
   end
 
diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb
index da9995345..f974914ac 100644
--- a/app/helpers/issues_helper.rb
+++ b/app/helpers/issues_helper.rb
@@ -294,6 +294,8 @@ module IssuesHelper
   # Returns the textual representation of a single journal detail
   def show_detail(detail, no_html=false, options={})
     multiple = false
+    show_diff = false
+
     case detail.property
     when 'attr'
       field = detail.prop_key.to_s.gsub(/\_id$/, "")
@@ -320,14 +322,21 @@ module IssuesHelper
       when 'is_private'
         value = l(detail.value == "0" ? :general_text_No : :general_text_Yes) unless detail.value.blank?
         old_value = l(detail.old_value == "0" ? :general_text_No : :general_text_Yes) unless detail.old_value.blank?
+
+      when 'description'
+        show_diff = true
       end
     when 'cf'
       custom_field = detail.custom_field
       if custom_field
-        multiple = custom_field.multiple?
         label = custom_field.name
-        value = format_value(detail.value, custom_field) if detail.value
-        old_value = format_value(detail.old_value, custom_field) if detail.old_value
+        if custom_field.format.class.change_as_diff
+          show_diff = true
+        else
+          multiple = custom_field.multiple?
+          value = format_value(detail.value, custom_field) if detail.value
+          old_value = format_value(detail.old_value, custom_field) if detail.old_value
+        end
       end
     when 'attachment'
       label = l(:label_attachment)
@@ -373,7 +382,7 @@ module IssuesHelper
       end
     end
 
-    if detail.property == 'attr' && detail.prop_key == 'description'
+    if show_diff
       s = l(:text_journal_changed_no_detail, :label => label)
       unless no_html
         diff_link = link_to 'diff',
diff --git a/lib/redmine/field_format.rb b/lib/redmine/field_format.rb
index 32a523227..68e59ffa5 100644
--- a/lib/redmine/field_format.rb
+++ b/lib/redmine/field_format.rb
@@ -70,6 +70,9 @@ module Redmine
       class_attribute :form_partial
       self.form_partial = nil
 
+      class_attribute :change_as_diff
+      self.change_as_diff = false
+
       def self.add(name)
         self.format_name = name
         Redmine::FieldFormat.add(name, self)
@@ -293,6 +296,7 @@ module Redmine
       add 'text'
       self.searchable_supported = true
       self.form_partial = 'custom_fields/formats/text'
+      self.change_as_diff = true
 
       def formatted_value(view, custom_field, value, customized=nil, html=false)
         if html
diff --git a/test/functional/journals_controller_test.rb b/test/functional/journals_controller_test.rb
index 142bbb05d..557fd3912 100644
--- a/test/functional/journals_controller_test.rb
+++ b/test/functional/journals_controller_test.rb
@@ -51,7 +51,7 @@ class JournalsControllerTest < ActionController::TestCase
     assert_not_include journal, assigns(:journals)
   end
 
-  def test_diff
+  def test_diff_for_description_change
     get :diff, :id => 3, :detail_id => 4
     assert_response :success
     assert_template 'diff'
@@ -60,6 +60,30 @@ class JournalsControllerTest < ActionController::TestCase
     assert_select 'span.diff_in', :text => /added/
   end
 
+  def test_diff_for_custom_field
+    field = IssueCustomField.create!(:name => "Long field", :field_format => 'text')
+    journal = Journal.create!(:journalized => Issue.find(2), :notes => 'Notes', :user_id => 1)
+    detail = JournalDetail.create!(:journal => journal, :property => 'cf', :prop_key => field.id,
+      :old_value => 'Foo', :value => 'Bar')
+
+    get :diff, :id => journal.id, :detail_id => detail.id
+    assert_response :success
+    assert_template 'diff'
+
+    assert_select 'span.diff_out', :text => /Foo/
+    assert_select 'span.diff_in', :text => /Bar/
+  end
+
+  def test_diff_for_custom_field_should_be_denied_if_custom_field_is_not_visible
+    field = IssueCustomField.create!(:name => "Long field", :field_format => 'text', :visible => false, :role_ids => [1])
+    journal = Journal.create!(:journalized => Issue.find(2), :notes => 'Notes', :user_id => 1)
+    detail = JournalDetail.create!(:journal => journal, :property => 'cf', :prop_key => field.id,
+      :old_value => 'Foo', :value => 'Bar')
+
+    get :diff, :id => journal.id, :detail_id => detail.id
+    assert_response 302
+  end
+
   def test_diff_should_default_to_description_diff
     get :diff, :id => 3
     assert_response :success
diff --git a/test/unit/helpers/issues_helper_test.rb b/test/unit/helpers/issues_helper_test.rb
index 35ebc2730..2062464fd 100644
--- a/test/unit/helpers/issues_helper_test.rb
+++ b/test/unit/helpers/issues_helper_test.rb
@@ -203,12 +203,25 @@ class IssuesHelperTest < ActionView::TestCase
     assert_match '6.30', show_detail(detail, true)
   end
 
+  test 'show_detail should not show values with a description attribute' do
+    detail = JournalDetail.new(:property => 'attr', :prop_key => 'description',
+                               :old_value => 'Foo', :value => 'Bar')
+    assert_equal 'Description updated', show_detail(detail, true)
+  end
+
   test 'show_detail should show old and new values with a custom field' do
     detail = JournalDetail.new(:property => 'cf', :prop_key => '1',
                                :old_value => 'MySQL', :value => 'PostgreSQL')
     assert_equal 'Database changed from MySQL to PostgreSQL', show_detail(detail, true)
   end
 
+  test 'show_detail should not show values with a long text custom field' do
+    field = IssueCustomField.create!(:name => "Long field", :field_format => 'text')
+    detail = JournalDetail.new(:property => 'cf', :prop_key => field.id,
+                               :old_value => 'Foo', :value => 'Bar')
+    assert_equal 'Long field updated', show_detail(detail, true)
+  end
+
   test 'show_detail should show added file' do
     detail = JournalDetail.new(:property => 'attachment', :prop_key => '1',
                                :old_value => nil, :value => 'error281.txt')