don't count children twice when summing estimated hours

app/models/queries/work_packages/columns/work_package_column.rb

@@ -42,12 +42,46 @@ class Queries::WorkPackages::Columns::WorkPackageColumn < Queries::Columns::Base
   end
 
   def sum_of(work_packages)
-    if work_packages.is_a?(Array)
+    wps = filter_for_sum work_packages
+
+    if wps.is_a?(Array)
       # TODO: Sums::grouped_sums might call through here without an AR::Relation
       # Ensure that this also calls using a Relation and drop this (slow!) implementation
-      work_packages.map { |wp| value(wp) }.compact.reduce(:+)
+      wps.map { |wp| value(wp) }.compact.reduce(:+)
+    else
+      wps.sum(name)
+    end
+  end
+
+  ##
+  # Sometimes we don't want to consider all work packages when calculating
+  # the sum for a certain column.
+  #
+  # Specifically we don't want to consider child work packages when summing up
+  # the estimated hours for work packages since the estimated hours of
+  # parent work packages already include those of their children.
+  #
+  # Right now we cover only this one case here explicilty.
+  # As soon as there are more cases to be considered this shall be
+  # refactored into something more generic and outside of this class.
+  def filter_for_sum(work_packages)
+    if name == :estimated_hours
+      filter_for_sum_of_estimated_hours work_packages
+    else
+      work_packages
+    end
+  end
+
+  def filter_for_sum_of_estimated_hours(work_packages)
+    # Why an array? See TODO above in #sum_of.
+    if work_packages.is_a? Array
+      work_packages.reject { |wp| !wp.children.empty? && work_packages.any? { |x| x.parent_id == wp.id } }
     else
-      work_packages.sum(name)
+      # @TODO Replace with CTE once we dropped MySQL support (MySQL only supports CTEs from version 8 onwards).
+      #       With a CTE (common table expression) we only need to query the work packages once and can then
+      #       drill the results down to the desired subset. Right now we run the work packages query a second
+      #       time in a sub query.
+      work_packages.without_children in: work_packages
     end
   end
 end

app/models/work_package/parent.rb

@@ -36,6 +36,56 @@ module WorkPackage::Parent
     base.virtual_attribute 'parent_id', cast_type: :integer
 
     base.define_attribute_method 'parent'
+
+    base.scope :with_parent, ->(*args) do
+      opts = Hash(args.first)
+      # noinspection RubySimplifyBooleanInspection
+      neg = opts[:present] == false ? "NOT" : ""
+      rel = Relation.table_name
+      wp = WorkPackage.table_name
+
+      query = "#{neg} EXISTS (SELECT 1 FROM #{rel} WHERE #{rel}.to_id = #{wp}.id AND #{rel}.hierarchy > 0"
+
+      if opts[:in].respond_to? :arel
+        subset = opts[:in].arel #                            .select() (or project()) will only add columns
+        subset.projections = [WorkPackage.arel_table[:id]] # but we only need the ID, so we reset the projections
+
+        query += " AND relations.from_id IN (#{subset.to_sql})"
+      end
+
+      query += " LIMIT 1)"
+
+      where(query)
+    end
+
+    base.scope :without_parent, ->(*args) do
+      with_parent Hash(args.first).merge(present: false)
+    end
+
+    base.scope :with_children, ->(*args) do
+      opts = Hash(args.first)
+      # noinspection RubySimplifyBooleanInspection
+      neg = opts[:present] == false ? "NOT" : ""
+      rel = Relation.table_name
+      wp = WorkPackage.table_name
+
+      query = "#{neg} EXISTS (SELECT 1 FROM #{rel} WHERE #{rel}.from_id = #{wp}.id AND #{rel}.hierarchy > 0"
+
+      if opts[:in].respond_to? :arel
+        subset = opts[:in].arel #                            .select() (or project()) will only add columns
+        subset.projections = [WorkPackage.arel_table[:id]] # but we only need the ID, so we reset the projections
+
+        query += " AND relations.to_id IN (#{subset.to_sql})"
+      end
+
+      query += " LIMIT 1)"
+
+      where(query)
+    end
+
+    base.scope :without_children, ->(*args) do
+      with_children Hash(args.first).merge(present: false)
+    end
   end
 
   attr_accessor :parent_object,
@@ -57,6 +107,10 @@ module WorkPackage::Parent
     end
   end
 
+  def has_parent?
+    !parent_relation.nil?
+  end
+
   def reload(*args)
     @parent_object = nil
 

spec/models/queries/work_packages/columns/work_package_column_spec.rb

@@ -36,4 +36,57 @@ describe Queries::WorkPackages::Columns::WorkPackageColumn, type: :model do
   it "allows to be constructed without attribute highlightable" do
     expect(described_class.new('foo').highlightable?).to eq(false)
   end
+
+  describe "sum of" do
+    describe :estimated_hours do
+      context "with work packages in a hierarchy" do
+        let(:work_packages) do
+          hierarchy = [
+            ["Single", 1],
+            {
+              ["Parent", 3] => [
+                ["Child 1 of Parent", 1],
+                ["Child 2 of Parent", 1],
+                ["Hidden Child 3 of Parent", 1]
+              ]
+            },
+            {
+              ["Hidden Parent", 4] => [
+                ["Child of Hidden Parent", 1],
+                ["Hidden Child", 3]
+              ]
+            },
+            {
+              ["Parent 2", 3] => [
+                ["Child 1 of Parent 2", 1],
+                {
+                  ["Nested Parent", 2] => [
+                    ["Child 1 of Nested Parent", 1],
+                    ["Child 2 of Nested Parent", 1]
+                  ]
+                }
+              ]
+            }
+          ]
+
+          build_work_package_hierarchy hierarchy, :subject, :estimated_hours
+        end
+
+        let(:result_set) { WorkPackage.where("NOT subject LIKE 'Hidden%'") }
+        let(:column) { Queries::WorkPackages::Columns::WorkPackageColumn.new :estimated_hours }
+
+        before do
+          work_packages # create work packages
+
+          expect(WorkPackage.count).to eq work_packages.size
+          expect(result_set.count).to eq(work_packages.size - 3) # all work packages except the hidden parent and children
+        end
+
+        it "yields the correct sum, not counting any children (of parents in the result set) twice" do
+          expect(column.sum_of(result_set)).to eq 7 # (Single + Child 1 of Parent + Child 2 of Parent + Child of Hidden Parent + Parent 2)
+          expect(column.sum_of(WorkPackage.all)).to eq 11 # (Single + Parent + Hidden Parent + Parent 2)
+        end
+      end
+    end
+  end
 end

spec/support/work_packages.rb

@@ -61,3 +61,24 @@ end
 def become_member_with_move_work_package_permissions
   become_member_with_permissions [:move_work_packages]
 end
+
+def build_work_package_hierarchy(data, *attributes, parent: nil)
+  work_packages = []
+
+  Array(data).each do |attr|
+    if attr.is_a? Hash
+      parent_wp = FactoryBot.create :work_package, **attributes.zip(attr.keys.first).to_h
+
+      work_packages << parent_wp
+      work_packages += build_work_package_hierarchy(attr.values.first, *attributes, parent: parent_wp)
+    else
+      wp = FactoryBot.create :work_package, **attributes.zip(attr).to_h
+
+      parent.children << wp if parent
+
+      work_packages << wp
+    end
+  end
+
+  work_packages
+end