Don't store total estimated hours on parent issues (#16092).

Parent can now have its own estimate that is summed up with children estimates.

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

7 files changed, 77 insertions, 44 deletions

diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb
index 62a65a66e..a8dcdca91 100644
--- a/app/helpers/issues_helper.rb
+++ b/app/helpers/issues_helper.rb
@@ -113,6 +113,14 @@ module IssuesHelper
     s.html_safe
   end
 
+  def issue_estimated_hours_details(issue)
+    s = issue.estimated_hours.present? ? l_hours(issue.estimated_hours) : ""
+    unless issue.leaf? || issue.total_estimated_hours.nil?
+      s << " (#{l(:label_total)}: #{l_hours(issue.total_estimated_hours)})"
+    end
+    s.html_safe
+  end
+
   # Returns an array of error messages for bulk edited issues
   def bulk_edit_error_messages(issues)
     messages = {}
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 2944031ac..85c3ff871 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -206,6 +206,7 @@ class Issue < ActiveRecord::Base
     @assignable_versions = nil
     @relations = nil
     @spent_hours = nil
+    @total_estimated_hours = nil
     base_reload(*args)
   end
 
@@ -435,9 +436,6 @@ class Issue < ActiveRecord::Base
     if done_ratio_derived?
       names -= %w(done_ratio)
     end
-    unless leaf?
-      names -= %w(estimated_hours)
-    end
     names
   end
 
@@ -929,6 +927,14 @@ class Issue < ActiveRecord::Base
         sum("#{TimeEntry.table_name}.hours").to_f || 0.0
   end
 
+  def total_estimated_hours
+    if leaf?
+      estimated_hours
+    else
+      @total_estimated_hours ||= self_and_descendants.sum(:estimated_hours)
+    end
+  end
+
   def relations
     @relations ||= IssueRelation::Relations.new(self, (relations_from + relations_to).sort)
   end
@@ -1488,10 +1494,6 @@ class Issue < ActiveRecord::Base
         end
       end
 
-      # estimate = sum of leaves estimates
-      p.estimated_hours = p.leaves.sum(:estimated_hours).to_f
-      p.estimated_hours = nil if p.estimated_hours == 0.0
-
       # ancestors will be recursively updated
       p.save(:validate => false)
     end
diff --git a/app/models/version.rb b/app/models/version.rb
index e4cbcc16c..b8c7b6f60 100644
--- a/app/models/version.rb
+++ b/app/models/version.rb
@@ -83,7 +83,7 @@ class Version < ActiveRecord::Base
   # Returns the total estimated time for this version
   # (sum of leaves estimated_hours)
   def estimated_hours
-    @estimated_hours ||= fixed_issues.leaves.sum(:estimated_hours).to_f
+    @estimated_hours ||= fixed_issues.sum(:estimated_hours).to_f
   end
 
   # Returns the total reported time for this version
diff --git a/app/views/issues/show.html.erb b/app/views/issues/show.html.erb
index ad8ee98a4..d8219f933 100644
--- a/app/views/issues/show.html.erb
+++ b/app/views/issues/show.html.erb
@@ -58,8 +58,8 @@
     rows.right l(:field_done_ratio), progress_bar(@issue.done_ratio, :width => '80px', :legend => "#{@issue.done_ratio}%"), :class => 'progress'
   end
   unless @issue.disabled_core_fields.include?('estimated_hours')
-    unless @issue.estimated_hours.nil?
-      rows.right l(:field_estimated_hours), l_hours(@issue.estimated_hours), :class => 'estimated-hours'
+    unless @issue.total_estimated_hours.nil?
+      rows.right l(:field_estimated_hours), issue_estimated_hours_details(@issue), :class => 'estimated-hours'
     end
   end
   if User.current.allowed_to?(:view_time_entries, @project)
diff --git a/db/migrate/20150525103953_clear_estimated_hours_on_parent_issues.rb b/db/migrate/20150525103953_clear_estimated_hours_on_parent_issues.rb
new file mode 100644
index 000000000..8eed815f7
--- /dev/null
+++ b/db/migrate/20150525103953_clear_estimated_hours_on_parent_issues.rb
@@ -0,0 +1,15 @@
+class ClearEstimatedHoursOnParentIssues < ActiveRecord::Migration
+  def self.up
+    # Clears estimated hours on parent issues
+    Issue.where("rgt > lft + 1 AND estimated_hours > 0").update_all :estimated_hours => nil
+  end
+
+  def self.down
+    table_name = Issue.table_name
+    leaves_sum_select = "SELECT SUM(leaves.estimated_hours) FROM #{table_name} leaves" +
+      " WHERE leaves.root_id = #{table_name}.root_id AND leaves.lft > #{table_name}.lft AND leaves.rgt < #{table_name}.rgt" +
+      " AND leaves.rgt = leaves.lft + 1"
+
+    Issue.where("rgt > lft + 1").update_all "estimated_hours = (#{leaves_sum_select})"
+  end
+end
diff --git a/test/unit/issue_nested_set_test.rb b/test/unit/issue_nested_set_test.rb
index 66bb2ba91..0c2631a27 100644
--- a/test/unit/issue_nested_set_test.rb
+++ b/test/unit/issue_nested_set_test.rb
@@ -287,26 +287,6 @@ class IssueNestedSetTest < ActiveSupport::TestCase
     end
   end
 
-  def test_parent_estimate_should_be_sum_of_leaves
-    parent = Issue.generate!
-    parent.generate_child!(:estimated_hours => nil)
-    assert_equal nil, parent.reload.estimated_hours
-    parent.generate_child!(:estimated_hours => 5)
-    assert_equal 5, parent.reload.estimated_hours
-    parent.generate_child!(:estimated_hours => 7)
-    assert_equal 12, parent.reload.estimated_hours
-  end
-
-  def test_move_parent_updates_old_parent_attributes
-    first_parent = Issue.generate!
-    second_parent = Issue.generate!
-    child = first_parent.generate_child!(:estimated_hours => 5)
-    assert_equal 5, first_parent.reload.estimated_hours
-    child.update_attributes(:estimated_hours => 7, :parent_issue_id => second_parent.id)
-    assert_equal 7, second_parent.reload.estimated_hours
-    assert_nil first_parent.reload.estimated_hours
-  end
-
   def test_project_copy_should_copy_issue_tree
     p = Project.create!(:name => 'Tree copy', :identifier => 'tree-copy', :tracker_ids => [1, 2])
     i1 = Issue.generate!(:project => p, :subject => 'i1')
diff --git a/test/unit/issue_subtasking_test.rb b/test/unit/issue_subtasking_test.rb
index d9f9a0824..6f00ee948 100644
--- a/test/unit/issue_subtasking_test.rb
+++ b/test/unit/issue_subtasking_test.rb
@@ -146,23 +146,41 @@ class IssueSubtaskingTest < ActiveSupport::TestCase
   end
 
   def test_done_ratio_of_parent_with_a_child_without_estimated_time_should_not_exceed_100
-    parent = Issue.generate!
-    parent.generate_child!(:estimated_hours => 40)
-    parent.generate_child!(:estimated_hours => 40)
-    parent.generate_child!(:estimated_hours => 20)
-    parent.generate_child!
-    parent.reload.children.each(&:close!)
-    assert_equal 100, parent.reload.done_ratio
+    with_settings :parent_issue_done_ratio => 'derived' do
+      parent = Issue.generate!
+      parent.generate_child!(:estimated_hours => 40)
+      parent.generate_child!(:estimated_hours => 40)
+      parent.generate_child!(:estimated_hours => 20)
+      parent.generate_child!
+      parent.reload.children.each(&:close!)
+      assert_equal 100, parent.reload.done_ratio
+    end
   end
 
   def test_done_ratio_of_parent_with_a_child_with_estimated_time_at_0_should_not_exceed_100
-    parent = Issue.generate!
-    parent.generate_child!(:estimated_hours => 40)
-    parent.generate_child!(:estimated_hours => 40)
-    parent.generate_child!(:estimated_hours => 20)
-    parent.generate_child!(:estimated_hours => 0)
-    parent.reload.children.each(&:close!)
-    assert_equal 100, parent.reload.done_ratio
+    with_settings :parent_issue_done_ratio => 'derived' do
+      parent = Issue.generate!
+      parent.generate_child!(:estimated_hours => 40)
+      parent.generate_child!(:estimated_hours => 40)
+      parent.generate_child!(:estimated_hours => 20)
+      parent.generate_child!(:estimated_hours => 0)
+      parent.reload.children.each(&:close!)
+      assert_equal 100, parent.reload.done_ratio
+    end
+  end
+
+  def test_changing_parent_should_update_previous_parent_done_ratio
+    with_settings :parent_issue_done_ratio => 'derived' do
+      first_parent = Issue.generate!
+      second_parent = Issue.generate!
+      first_parent.generate_child!(:done_ratio => 40)
+      child = first_parent.generate_child!(:done_ratio => 20)
+      assert_equal 30, first_parent.reload.done_ratio
+      assert_equal 0, second_parent.reload.done_ratio
+      child.update_attributes(:parent_issue_id => second_parent.id)
+      assert_equal 40,  first_parent.reload.done_ratio
+      assert_equal 20, second_parent.reload.done_ratio
+    end
   end
 
   def test_parent_dates_should_be_editable_with_parent_issue_dates_set_to_independent
@@ -227,4 +245,14 @@ class IssueSubtaskingTest < ActiveSupport::TestCase
       assert_equal 0, parent.reload.done_ratio
     end
   end
+
+  def test_parent_total_estimated_hours_should_be_sum_of_descendants
+    parent = Issue.generate!
+    parent.generate_child!(:estimated_hours => nil)
+    assert_equal 0, parent.reload.total_estimated_hours
+    parent.generate_child!(:estimated_hours => 5)
+    assert_equal 5, parent.reload.total_estimated_hours
+    parent.generate_child!(:estimated_hours => 7)
+    assert_equal 12, parent.reload.total_estimated_hours
+  end
 end