fix summing custom fields on wp table

Before the fix, string values where returned and then the `+` method was called which lead to the strings being concatenated. A subsequent call of `integer?` failed. The fix performs the aggregation in the database which is not only correct then but can also be carried out considerably faster. The drawback is, that the casting breaks the abstraction introduced by the cf strategies
pull/7847/head
ulferts 5 years ago
parent 8f3ad5af6c
commit aa3698d061
No known key found for this signature in database
GPG Key ID: A205708DE1284017
  1. 17
      app/models/queries/work_packages/columns/custom_field_column.rb
  2. 5
      app/models/query/sums.rb
  3. 49
      spec/features/work_packages/index_sums_spec.rb

@ -75,17 +75,20 @@ class Queries::WorkPackages::Columns::CustomFieldColumn < Queries::WorkPackages:
def sum_of(work_packages) def sum_of(work_packages)
if work_packages.respond_to?(:joins) if work_packages.respond_to?(:joins)
# we can't perform the aggregation on the SQL side. Try to filter useless rows to reduce work. cast = @cf.field_format == 'int' ? 'INTEGER' : 'FLOAT'
work_packages = work_packages
.joins(:custom_values)
.where(custom_values: { custom_field: @cf })
.where("#{CustomValue.table_name}.value IS NOT NULL")
.where("#{CustomValue.table_name}.value != ''")
end
CustomValue
.where(customized: work_packages, custom_field: @cf)
.where.not(value: nil)
.where.not(value: '')
.pluck("SUM(value::#{cast})")
.first
else
# TODO: eliminate calls of this method with an Array and drop the :compact call below # TODO: eliminate calls of this method with an Array and drop the :compact call below
ActiveSupport::Deprecation.warn('Passing an array of work packages is deprecated. Pass an AR-relation instead.')
work_packages.map { |wp| value(wp) }.compact.reduce(:+) work_packages.map { |wp| value(wp) }.compact.reduce(:+)
end end
end
def self.instances(context = nil) def self.instances(context = nil)
if context if context

@ -100,7 +100,8 @@ module ::Query::Sums
def crunch(num) def crunch(num)
return num if num.nil? or num.integer? return num if num.nil? or num.integer?
Float(format '%.2f', num.to_f)
Float(format('%.2f', num.to_f))
end end
def group_for_issue(issue = @current_issue) def group_for_issue(issue = @current_issue)
@ -121,7 +122,7 @@ module ::Query::Sums
def all_total_sums def all_total_sums
query.available_columns.select { |column| query.available_columns.select { |column|
column.summable? && Setting.work_package_list_summable_columns.include?(column.name.to_s) should_be_summed_up?(column)
}.inject({}) { |result, column| }.inject({}) { |result, column|
sum = total_sum_of(column) sum = total_sum_of(column)
result[column] = sum unless sum.nil? result[column] = sum unless sum.nil?

@ -29,18 +29,35 @@
require 'spec_helper' require 'spec_helper'
RSpec.feature 'Work package index sums', js: true do RSpec.feature 'Work package index sums', js: true do
let(:admin) { FactoryBot.create(:admin) } let(:admin) { FactoryBot.create(:admin) }
let(:project) { let(:project) do
FactoryBot.create(:project, name: 'project1', identifier: 'project1') FactoryBot.create(:project, name: 'project1', identifier: 'project1')
} end
let(:type) { FactoryBot.create(:type) }
let!(:work_package_1) { let!(:int_cf) do
FactoryBot.create(:work_package, project: project, estimated_hours: 10) FactoryBot.create(:int_wp_custom_field).tap do |cf|
} project.work_package_custom_fields << cf
let!(:work_package_2) { type.custom_fields << cf
FactoryBot.create(:work_package, project: project, estimated_hours: 15) end
} end
let!(:float_cf) do
FactoryBot.create(:float_wp_custom_field).tap do |cf|
project.work_package_custom_fields << cf
type.custom_fields << cf
end
end
let!(:work_package_1) do
FactoryBot.create(:work_package, project: project, type: type, estimated_hours: 10).tap do |wp|
wp.custom_field_values = { int_cf.id => 5, float_cf.id => 5.5 }
wp.save!
end
end
let!(:work_package_2) do
FactoryBot.create(:work_package, project: project, type: type, estimated_hours: 15).tap do |wp|
wp.custom_field_values = { int_cf.id => 7, float_cf.id => 7.7 }
wp.save!
end
end
let(:wp_table) { Pages::WorkPackagesTable.new(project) } let(:wp_table) { Pages::WorkPackagesTable.new(project) }
let(:columns) { ::Components::WorkPackages::Columns.new } let(:columns) { ::Components::WorkPackages::Columns.new }
@ -49,6 +66,10 @@ RSpec.feature 'Work package index sums', js: true do
before do before do
login_as(admin) login_as(admin)
allow(Setting)
.to receive(:work_package_list_summable_columns)
.and_return(%W(estimated_hours cf_#{int_cf.id} cf_#{float_cf.id}))
visit project_work_packages_path(project) visit project_work_packages_path(project)
expect(current_path).to eq('/projects/project1/work_packages') expect(current_path).to eq('/projects/project1/work_packages')
end end
@ -58,6 +79,10 @@ RSpec.feature 'Work package index sums', js: true do
# Add estimated time column # Add estimated time column
columns.add 'Estimated time' columns.add 'Estimated time'
# Add int cf column
columns.add int_cf.name
# Add float cf column
columns.add float_cf.name
# Trigger action from action menu dropdown # Trigger action from action menu dropdown
modal.set_display_sums enable: true modal.set_display_sums enable: true
@ -66,6 +91,8 @@ RSpec.feature 'Work package index sums', js: true do
expect(page).to have_selector('.wp-table--sum-container', text: 'Sum') expect(page).to have_selector('.wp-table--sum-container', text: 'Sum')
expect(page).to have_selector('.wp-table--sum-container', text: '25') expect(page).to have_selector('.wp-table--sum-container', text: '25')
expect(page).to have_selector('.wp-table--sum-container', text: '12')
expect(page).to have_selector('.wp-table--sum-container', text: '13.2')
# Update the sum # Update the sum
edit_field = wp_table.edit_field(work_package_1, :estimatedTime) edit_field = wp_table.edit_field(work_package_1, :estimatedTime)
@ -73,5 +100,7 @@ RSpec.feature 'Work package index sums', js: true do
expect(page).to have_selector('.wp-table--sum-container', text: 'Sum') expect(page).to have_selector('.wp-table--sum-container', text: 'Sum')
expect(page).to have_selector('.wp-table--sum-container', text: '35') expect(page).to have_selector('.wp-table--sum-container', text: '35')
expect(page).to have_selector('.wp-table--sum-container', text: '12')
expect(page).to have_selector('.wp-table--sum-container', text: '13.2')
end end
end end

Loading…
Cancel
Save