Avoid lots of CustomField.find_by_id calls when displaying an issue history with custom fields (#15072).

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

app/controllers/issues_controller.rb

@@ -103,6 +103,7 @@ class IssuesController < ApplicationController
     @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)
+    Journal.preload_journals_details_custom_fields(@journals)
     # TODO: use #select! when ruby1.8 support is dropped
     @journals.reject! {|journal| !journal.notes? && journal.visible_details.empty?}
     @journals.reverse! if User.current.wants_comments_in_reverse_order?

app/helpers/issues_helper.rb

@@ -261,23 +261,23 @@ module IssuesHelper
     values_by_field = {}
     details.each do |detail|
       if detail.property == 'cf'
-        field_id = detail.prop_key
-        field = CustomField.find_by_id(field_id)
+        field = detail.custom_field
         if field && field.multiple?
-          values_by_field[field_id] ||= {:added => [], :deleted => []}
+          values_by_field[field] ||= {:added => [], :deleted => []}
           if detail.old_value
-            values_by_field[field_id][:deleted] << detail.old_value
+            values_by_field[field][:deleted] << detail.old_value
           end
           if detail.value
-            values_by_field[field_id][:added] << detail.value
+            values_by_field[field][:added] << detail.value
           end
           next
         end
       end
       strings << show_detail(detail, no_html, options)
     end
-    values_by_field.each do |field_id, changes|
-      detail = JournalDetail.new(:property => 'cf', :prop_key => field_id)
+    values_by_field.each do |field, changes|
+      detail = JournalDetail.new(:property => 'cf', :prop_key => field.id.to_s)
+      detail.instance_variable_set "@custom_field", field
       if changes[:added].any?
         detail.value = changes[:added]
         strings << show_detail(detail, no_html, options)
@@ -320,7 +320,7 @@ module IssuesHelper
         old_value = l(detail.old_value == "0" ? :general_text_No : :general_text_Yes) unless detail.old_value.blank?
       end
     when 'cf'
-      custom_field = CustomField.find_by_id(detail.prop_key)
+      custom_field = detail.custom_field
       if custom_field
         multiple = custom_field.multiple?
         label = custom_field.name

app/models/journal.rb

@@ -58,9 +58,7 @@ class Journal < ActiveRecord::Base
   def visible_details(user=User.current)
     details.select do |detail|
       if detail.property == 'cf'
-        field_id = detail.prop_key
-        field = CustomField.find_by_id(field_id)
-        field && field.visible_by?(project, user)
+        detail.custom_field && detail.custom_field.visible_by?(project, user)
       elsif detail.property == 'relation'
         Issue.find_by_id(detail.value || detail.old_value).try(:visible?, user)
       else
@@ -146,6 +144,22 @@ class Journal < ActiveRecord::Base
     notified_watchers.map(&:mail)
   end
 
+  # Sets @custom_field instance variable on journals details using a single query
+  def self.preload_journals_details_custom_fields(journals)
+    field_ids = journals.map(&:details).flatten.select {|d| d.property == 'cf'}.map(&:prop_key).uniq
+    if field_ids.any?
+      fields_by_id = CustomField.find_all_by_id(field_ids).inject({}) {|h, f| h[f.id] = f; h}
+      journals.each do |journal|
+        journal.details.each do |detail|
+          if detail.property == 'cf'
+            detail.instance_variable_set "@custom_field", fields_by_id[detail.prop_key.to_i]
+          end
+        end
+      end
+    end
+    journals
+  end
+
   private
 
   def split_private_notes

app/models/journal_detail.rb

@@ -19,6 +19,12 @@ class JournalDetail < ActiveRecord::Base
   belongs_to :journal
   before_save :normalize_values
 
+  def custom_field
+    if property == 'cf'
+      @custom_field ||= CustomField.find_by_id(prop_key)
+    end
+  end
+
   private
 
   def normalize_values

test/unit/journal_test.rb

@@ -155,6 +155,20 @@ class JournalTest < ActiveSupport::TestCase
     assert journals.detect {|journal| !journal.issue.project.is_public?}
   end
 
+  def test_preload_journals_details_custom_fields_should_set_custom_field_instance_variable
+    d = JournalDetail.new(:property => 'cf', :prop_key => '2')
+    journals = [Journal.new(:details => [d])]
+
+    d.expects(:instance_variable_set).with("@custom_field", CustomField.find(2)).once
+    Journal.preload_journals_details_custom_fields(journals)
+  end
+
+  def test_preload_journals_details_custom_fields_with_empty_set
+    assert_nothing_raised do
+      Journal.preload_journals_details_custom_fields([])
+    end
+  end
+
   def test_details_should_normalize_dates
     j = JournalDetail.create!(:old_value => Date.parse('2012-11-03'), :value => Date.parse('2013-01-02'))
     j.reload
@@ -176,6 +190,16 @@ class JournalTest < ActiveSupport::TestCase
     assert_equal '0', j.value
   end
 
+  def test_custom_field_should_return_custom_field_for_cf_detail
+    d = JournalDetail.new(:property => 'cf', :prop_key => '2')
+    assert_equal CustomField.find(2), d.custom_field
+  end
+
+  def test_custom_field_should_return_nil_for_non_cf_detail
+    d = JournalDetail.new(:property => 'subject')
+    assert_equal nil, d.custom_field
+  end
+
   def test_visible_details_should_include_relations_to_visible_issues_only
     issue = Issue.generate!
     visible_issue = Issue.generate!