[#40867] Wrong progress value in parent task - Estimated hours

https://community.openproject.org/work_packages/40867
This is a second commit/pull request related to this bug.
It deals with the dependency of done_ratio on estimated_hours.
We need to recalculate done_ration along the hierarchy with
changes in estimated_hours.

app/services/work_packages/update_ancestors_service.rb

@@ -91,8 +91,16 @@ class WorkPackages::UpdateAncestorsService
   def inherit_attributes(ancestor, attributes)
     return unless attributes_justify_inheritance?(attributes)
 
+    # Estimated hours need to be calculated before the done_ratio below.
+    # The aggregation only depends on estimated hours.
     derive_estimated_hours(ancestor) if inherit?(attributes, :estimated_hours)
-    inherit_done_ratio(ancestor) if inherit?(attributes, :done_ratio)
+
+    # Progress (done_ratio or also: percentDone) depends on both
+    # the completion of sub-WPs, as well as the estimated hours
+    # as a weight factor. So changes in estimated hours also have
+    # to trigger a recalculation of done_ratio.
+    #
+    inherit_done_ratio(ancestor) if inherit?(attributes, :done_ratio) || inherit?(attributes, :estimated_hours)
   end
 
   def inherit?(attributes, attribute)

spec/services/work_packages/update_ancestors_est_spec.rb

@@ -1,86 +0,0 @@
-require 'spec_helper'
-
-describe WorkPackages::UpdateAncestorsService, type: :model, with_mail: false do
-  let(:user) { create :user }
-  let(:estimated_hours) { [nil, nil, nil] }
-  let(:done_ratios) { [0, 0, 0] }
-  let(:statuses) { %i(open open open) }
-  let(:open_status) { create :status }
-  let(:closed_status) { create :closed_status }
-  let(:aggregate_done_ratio) { 0.0 }
-
-  context 'with a common ancestor' do
-    let(:status) { open_status }
-    let(:done_ratio) { 50 }
-    let(:estimated_hours) { 7.0 }
-
-    let!(:grandparent) do
-      create :work_package,
-             derived_estimated_hours: estimated_hours,
-             done_ratio: done_ratio
-    end
-    let!(:old_parent) do
-      create :work_package,
-             parent: grandparent,
-             derived_estimated_hours: estimated_hours,
-             done_ratio: done_ratio
-    end
-    let!(:new_parent) do
-      create :work_package,
-             parent: grandparent
-    end
-    let!(:work_package) do
-      create :work_package,
-             parent: old_parent,
-             status: status,
-             estimated_hours: estimated_hours,
-             done_ratio: done_ratio
-    end
-
-    subject do
-      # binding.pry
-      work_package.parent = new_parent
-      # In this test case, derived_estimated_hours and done_ratio will not
-      # inherently change on grandparent.  However, if work_package has siblings
-      # then changing its parent could cause derived_estimated_hours and/or
-      # done_ratio on grandparent to inherently change.  To verify that
-      # grandparent can be properly updated in that case without making this
-      # test dependent on the implementation details of the
-      # derived_estimated_hours and done_ratio calculations, force
-      # derived_estimated_hours and done_ratio to change at the same time as the
-      # parent.
-      work_package.estimated_hours = (estimated_hours + 1)
-      work_package.done_ratio = (done_ratio + 1)
-      work_package.save!
-
-      described_class
-        .new(user: user,
-             work_package: work_package)
-        .call(%i(parent))
-    end
-
-    before do
-      subject
-    end
-
-    it 'is successful' do
-      expect(subject)
-        .to be_success
-    end
-
-    it 'returns both the former and new ancestors in the dependent results without duplicates' do
-      expect(subject.dependent_results.map(&:result))
-        .to match_array [new_parent, grandparent, old_parent]
-    end
-
-    it 'updates the done_ratio of the former parent' do
-      expect(old_parent.reload(select: :done_ratio).done_ratio)
-        .to be 0
-    end
-
-    it 'updates the estimated_hours of the former parent' do
-      expect(old_parent.reload(select: :derived_estimated_hours).derived_estimated_hours)
-        .to be_nil
-    end
-  end
-end

spec/services/work_packages/update_ancestors_service_spec.rb

@@ -76,10 +76,14 @@ describe WorkPackages::UpdateAncestorsService, type: :model, with_mail: false do
     let(:parent) { create :work_package, status: open_status }
 
     subject do
+      # In the call we only use estimated_hours (instead of also adding
+      # done_ratio) in order to test that changes in estimated hours
+      # trigger a recalculation of done_ration, because estimated hours
+      # act as weights in this calculation.
       described_class
         .new(user: user,
              work_package: children.first)
-        .call(%i(done_ratio estimated_hours))
+        .call(%i(estimated_hours))
     end
 
     context 'with no estimated hours and no progress' do