diff --git a/app/models/queries/work_packages/columns/property_column.rb b/app/models/queries/work_packages/columns/property_column.rb index 79f080ec1b..51d4f50f6c 100644 --- a/app/models/queries/work_packages/columns/property_column.rb +++ b/app/models/queries/work_packages/columns/property_column.rb @@ -43,7 +43,7 @@ class Queries::WorkPackages::Columns::PropertyColumn < Queries::WorkPackages::Co project: { association: 'project', sortable: "name", - groupable: true + groupable: 'project_id' }, subject: { sortable: "#{WorkPackage.table_name}.subject" @@ -51,7 +51,7 @@ class Queries::WorkPackages::Columns::PropertyColumn < Queries::WorkPackages::Co type: { association: 'type', sortable: "position", - groupable: true + groupable: 'type_id' }, parent: { association: 'ancestors_relations', @@ -62,35 +62,35 @@ class Queries::WorkPackages::Columns::PropertyColumn < Queries::WorkPackages::Co association: 'status', sortable: "position", highlightable: true, - groupable: true + groupable: 'status_id' }, priority: { association: 'priority', sortable: "position", default_order: 'desc', highlightable: true, - groupable: true + groupable: 'priority_id' }, author: { association: 'author', sortable: ["lastname", "firstname", "id"], - groupable: true + groupable: 'author_id' }, assigned_to: { association: 'assigned_to', sortable: ["lastname", "firstname", "id"], - groupable: true + groupable: 'assigned_to_id' }, responsible: { association: 'responsible', sortable: ["lastname", "firstname", "id"], - groupable: true + groupable: 'responsible_id' }, updated_at: { sortable: "#{WorkPackage.table_name}.updated_at", @@ -99,13 +99,13 @@ class Queries::WorkPackages::Columns::PropertyColumn < Queries::WorkPackages::Co category: { association: 'category', sortable: "name", - groupable: true + groupable: 'category_id' }, fixed_version: { association: 'fixed_version', sortable: ["name"], default_order: 'desc', - groupable: true + groupable: 'fixed_version_id' }, start_date: { # Put empty start_dates in the far future rather than in the far past diff --git a/app/models/query.rb b/app/models/query.rb index 8dcff66243..5b550af547 100644 --- a/app/models/query.rb +++ b/app/models/query.rb @@ -188,6 +188,7 @@ class Query < ActiveRecord::Base def add_short_filter(field, expression) return unless expression + parms = expression.scan(/\A(o|c|!\*|!|\*)?(.*)\z/).first add_filter field, (parms[0] || '='), [parms[1] || ''] end @@ -372,9 +373,8 @@ class Query < ActiveRecord::Base end # Returns the result set - # Valid options are :order, :include, :conditions - def results(options = {}) - Results.new(self, options) + def results + Results.new(self) end # Returns the journals diff --git a/app/models/query/group_by.rb b/app/models/query/group_by.rb index c92ce540e2..b242974af8 100644 --- a/app/models/query/group_by.rb +++ b/app/models/query/group_by.rb @@ -1,4 +1,5 @@ #-- encoding: UTF-8 + #-- copyright # OpenProject is a project management system. # Copyright (C) 2012-2018 the OpenProject Foundation (OPF) @@ -32,7 +33,7 @@ module ::Query::Grouping def work_package_count_by_group @work_package_count_by_group ||= begin if query.grouped? - r = groups_grouped_by_column + r = group_counts_by_group transform_group_keys(r) end @@ -45,33 +46,58 @@ module ::Query::Grouping work_package_count_by_group[group] end - def groups_grouped_by_column - # Rails will raise an (unexpected) RecordNotFound if there's only a nil group value - WorkPackage - .group(query.group_by_statement) + private + + def group_counts_by_group + work_packages_with_includes_for_count + .group(group_by_for_count) .visible - .includes(all_includes) - .joins(all_filter_joins) .references(:statuses, :projects) .where(query.statement) - .count - rescue ActiveRecord::RecordNotFound - { nil => work_package_count } + .order(order_for_count) + .pluck(pluck_for_count) + .to_h + end + + def work_packages_with_includes_for_count + WorkPackage + .includes(all_includes) + .joins(all_filter_joins) + end + + def group_by_for_count + Array(query.group_by_statement).map { |statement| Arel.sql(statement) } + + [Arel.sql(group_by_sort(false))] + end + + def pluck_for_count + Array(query.group_by_statement).map { |statement| Arel.sql(statement) } + + [Arel.sql('COUNT(DISTINCT "work_packages"."id")')] + end + + def order_for_count + Arel.sql(group_by_sort) end def transform_group_keys(groups) - column = query.group_by_column + if query.group_by_column.is_a?(Queries::WorkPackages::Columns::CustomFieldColumn) + transform_custom_field_keys(groups) + else + transform_property_keys(groups) + end + end - if column.is_a?(Queries::WorkPackages::Columns::CustomFieldColumn) && column.custom_field.list? - transform_list_group_by_keys(column.custom_field, groups) - elsif column.is_a?(Queries::WorkPackages::Columns::CustomFieldColumn) - transform_custom_field_keys(column.custom_field, groups) + def transform_custom_field_keys(groups) + custom_field = query.group_by_column.custom_field + + if custom_field.list? + transform_list_custom_field_keys(custom_field, groups) else - groups + transform_single_custom_field_keys(custom_field, groups) end end - def transform_list_group_by_keys(custom_field, groups) + def transform_list_custom_field_keys(custom_field, groups) options = custom_options_for_keys(custom_field, groups) groups.transform_keys do |key| @@ -96,25 +122,52 @@ module ::Query::Grouping custom_field.custom_options.find(keys.flatten.uniq).group_by { |o| o.id.to_s } end - def transform_custom_field_keys(custom_field, groups) + def transform_single_custom_field_keys(custom_field, groups) groups.transform_keys { |key| custom_field.cast_value(key) } end + + def transform_property_keys(groups) + association = WorkPackage.reflect_on_all_associations.detect { |a| a.name == query.group_by_column.name.to_sym } + + if association + transform_association_property_keys(association, groups) + else + groups + end + end + + def transform_association_property_keys(association, groups) + ar_keys = association.class_name.constantize.find(groups.keys) + + groups.map do |key, value| + [ar_keys.detect { |ar_key| ar_key.id == key }, value] + end.to_h + end + # Returns the SQL sort order that should be prepended for grouping - def group_by_sort_order + def group_by_sort(order = true) if query.grouped? && (column = query.group_by_column) aliases = include_aliases Array(column.sortable).map do |s| - aliased_group_by_sort_order(s, order_for_group_by(column), aliases[column.name]) - end.join(',') + direction = order ? order_for_group_by(column) : nil + + aliased_group_by_sort_order(aliases[column.name], s, direction) + end.join(', ') end end - def aliased_group_by_sort_order(sortable, order, alias_name) - if alias_name - "#{alias_name}.#{sortable} #{order}" + def aliased_group_by_sort_order(alias_name, sortable, order = nil) + column = if alias_name + "#{alias_name}.#{sortable}" + else + sortable + end + + if order + column + " #{order}" else - "#{sortable} #{order}" + column end end diff --git a/app/models/query/results.rb b/app/models/query/results.rb index 7a51d79580..05f4055dfb 100644 --- a/app/models/query/results.rb +++ b/app/models/query/results.rb @@ -36,12 +36,9 @@ class ::Query::Results include ::Query::Sums include Redmine::I18n - attr_accessor :options, - :query + attr_accessor :query - # Valid options are :order, :include, :conditions - def initialize(query, options = {}) - self.options = options + def initialize(query) self.query = query end @@ -60,7 +57,6 @@ class ::Query::Results def work_packages work_package_scope .where(query.statement) - .where(options[:conditions]) .includes(all_includes) .joins(all_joins) .order(order_option) @@ -78,7 +74,6 @@ class ::Query::Results def versions scope = Version .visible - .where(options[:conditions]) if query.project scope.where(query.project_limiting_filter.where) @@ -88,7 +83,7 @@ class ::Query::Results end def order_option - order_option = [group_by_sort_order].reject(&:blank?).join(', ') + order_option = [group_by_sort].reject(&:blank?).join(', ') if order_option.blank? nil @@ -107,8 +102,7 @@ class ::Query::Results def all_includes (%i(status project) + - includes_for_columns(include_columns) + - (options[:include] || [])).uniq + includes_for_columns(include_columns)).uniq end def all_joins diff --git a/spec/models/query/results_spec.rb b/spec/models/query/results_spec.rb index f3d16f81aa..70e5ae1ada 100644 --- a/spec/models/query/results_spec.rb +++ b/spec/models/query/results_spec.rb @@ -28,21 +28,13 @@ require 'spec_helper' -describe ::Query::Results, type: :model do +describe ::Query::Results, type: :model, with_mail: false do let(:query) do FactoryBot.build :query, show_hierarchies: false end let(:query_results) do - ::Query::Results.new query, - include: %i( - assigned_to - type - priority - category - fixed_version - ), - order: 'work_packages.root_id DESC, work_packages.lft ASC' + ::Query::Results.new query end let(:project_1) { FactoryBot.create :project } let(:role_pm) do @@ -77,10 +69,27 @@ describe ::Query::Results, type: :model do let(:query) do FactoryBot.build :query, show_hierarchies: false, - group_by: group_by + group_by: group_by, + project: project_1 + end + let(:type_1) do + FactoryBot.create(:type) + end + let(:type_2) do + FactoryBot.create(:type) + end + let(:work_package1) do + FactoryBot.create(:work_package, + type: type_1, + project: project_1) + end + let(:work_package2) do + FactoryBot.create(:work_package, + type: type_2, + project: project_1) end - context 'when grouping by responsible' do + context 'grouping by responsible' do let(:group_by) { 'responsible' } it 'should produce a valid SQL statement' do @@ -88,7 +97,7 @@ describe ::Query::Results, type: :model do end end - context 'when grouping and filtering by text' do + context 'grouping and filtering by text' do let(:group_by) { 'responsible' } before do @@ -99,6 +108,175 @@ describe ::Query::Results, type: :model do expect { query_results.work_package_count_by_group }.not_to raise_error end end + + context 'grouping by type' do + let(:group_by) { 'type' } + + before do + work_package1 + work_package2 + + login_as(user_1) + end + + it 'returns the groups sorted by type`s position' do + type_1.update_column(:position, 1) + type_2.update_column(:position, 2) + + result = query_results.work_package_count_by_group + + expect(result.length) + .to eql 2 + + expect(result.keys.map(&:id)) + .to eql [type_1.id, type_2.id] + + type_1.update_column(:position, 2) + type_2.update_column(:position, 1) + + new_results = ::Query::Results.new(query) + + result = new_results.work_package_count_by_group + + expect(result.length) + .to eql 2 + + expect(result.keys.map(&:id)) + .to eql [type_2.id, type_1.id] + end + end + + context 'grouping by list custom field and filtering for it at the same time' do + let!(:custom_field) do + FactoryBot.create(:list_wp_custom_field, + is_for_all: true, + is_filter: true, + multi_value: true).tap do |cf| + work_package1.type.custom_fields << cf + work_package2.type.custom_fields << cf + end + end + let(:first_value) do + custom_field.custom_options.first + end + let(:last_value) do + custom_field.custom_options.last + end + let(:group_by) { "cf_#{custom_field.id}" } + + before do + login_as(user_1) + + work_package1.send(:"custom_field_#{custom_field.id}=", first_value) + work_package1.save! + work_package2.send(:"custom_field_#{custom_field.id}=", [first_value, + last_value]) + work_package2.save! + end + + it 'yields no error but rather returns the result' do + expect { query_results.work_package_count_by_group }.not_to raise_error + + group_count = query_results.work_package_count_by_group + expected_groups = [[first_value], [first_value, last_value]] + + group_count.each do |key, count| + expect(count).to eql 1 + expect(expected_groups.any? { |group| group & key == key & group }).to be_truthy + end + end + end + + context 'grouping by int custom field' do + let!(:custom_field) do + FactoryBot.create(:int_wp_custom_field, is_for_all: true, is_filter: true) + end + + let(:group_by) { "cf_#{custom_field.id}" } + + before do + login_as(user_1) + + wp_p1[0].type.custom_fields << custom_field + project_1.work_package_custom_fields << custom_field + + wp_p1[0].update_attribute(:"custom_field_#{custom_field.id}", 42) + wp_p1[0].save + wp_p1[1].update_attribute(:"custom_field_#{custom_field.id}", 42) + wp_p1[1].save + end + + it 'returns a hash of counts by value' do + expect(query_results.work_package_count_by_group).to eql(42 => 2, nil => 1) + end + end + + context 'grouping by user custom field' do + let!(:custom_field) do + FactoryBot.create(:user_wp_custom_field, is_for_all: true, is_filter: true) + end + + let(:group_by) { "cf_#{custom_field.id}" } + + before do + login_as(user_1) + + wp_p1[0].type.custom_fields << custom_field + project_1.work_package_custom_fields << custom_field + end + + it 'returns nil as user custom fields are not groupable' do + expect(query_results.work_package_count_by_group).to be_nil + end + end + + context 'grouping by bool custom field' do + let!(:custom_field) do + FactoryBot.create(:bool_wp_custom_field, is_for_all: true, is_filter: true) + end + + let(:group_by) { "cf_#{custom_field.id}" } + + before do + login_as(user_1) + + wp_p1[0].type.custom_fields << custom_field + project_1.work_package_custom_fields << custom_field + + wp_p1[0].update_attribute(:"custom_field_#{custom_field.id}", true) + wp_p1[0].save + wp_p1[1].update_attribute(:"custom_field_#{custom_field.id}", true) + wp_p1[1].save + end + + it 'returns a hash of counts by value' do + expect(query_results.work_package_count_by_group).to eql(true => 2, nil => 1) + end + end + + context 'grouping by date custom field' do + let!(:custom_field) do + FactoryBot.create(:date_wp_custom_field, is_for_all: true, is_filter: true) + end + + let(:group_by) { "cf_#{custom_field.id}" } + + before do + login_as(user_1) + + wp_p1[0].type.custom_fields << custom_field + project_1.work_package_custom_fields << custom_field + + wp_p1[0].update_attribute(:"custom_field_#{custom_field.id}", Date.today) + wp_p1[0].save + wp_p1[1].update_attribute(:"custom_field_#{custom_field.id}", Date.today) + wp_p1[1].save + end + + it 'returns a hash of counts by value' do + expect(query_results.work_package_count_by_group).to eql(Date.today => 2, nil => 1) + end + end end describe '#work_packages' do @@ -587,85 +765,4 @@ describe ::Query::Results, type: :model do end end end - - context 'when grouping by custom field' do - let!(:custom_field) do - FactoryBot.create(:int_wp_custom_field, is_for_all: true, is_filter: true) - end - - before do - allow(User).to receive(:current).and_return(user_1) - - wp_p1[0].type.custom_fields << custom_field - project_1.work_package_custom_fields << custom_field - - wp_p1[0].update_attribute(:"custom_field_#{custom_field.id}", 42) - wp_p1[0].save - wp_p1[1].update_attribute(:"custom_field_#{custom_field.id}", 42) - wp_p1[1].save - - query.project = project_1 - query.group_by = "cf_#{custom_field.id}" - end - - describe '#work_package_count_by_group' do - it 'returns a hash of counts by value' do - expect(query.results.work_package_count_by_group).to eql(42 => 2, nil => 1) - end - end - end - - context 'when grouping by list custom field and filtering for it at the same time' do - let!(:custom_field) do - FactoryBot.create(:list_wp_custom_field, - is_for_all: true, - is_filter: true, - multi_value: true).tap do |cf| - work_package1.type.custom_fields << cf - end - end - let(:first_value) do - custom_field.custom_options.first - end - let(:last_value) do - custom_field.custom_options.last - end - - let(:work_package1) do - FactoryBot.create(:work_package, - project: project_1) - end - let(:work_package2) do - FactoryBot.create(:work_package, - type: work_package1.type, - project: project_1) - end - - before do - allow(User).to receive(:current).and_return(user_1) - - query.group_by = "cf_#{custom_field.id}" - query.project = project_1 - - work_package1.send(:"custom_field_#{custom_field.id}=", first_value) - work_package1.save! - work_package2.send(:"custom_field_#{custom_field.id}=", [first_value, - last_value]) - work_package2.save! - end - - describe '#work_package_count_by_group' do - it 'yields no error but rather returns the result' do - expect { query.results.work_package_count_by_group }.not_to raise_error - - group_count = query.results.work_package_count_by_group - expected_groups = [[first_value], [first_value, last_value]] - - group_count.each do |key, count| - expect(count).to eql 1 - expect(expected_groups.any? { |group| group & key == key & group }).to be_truthy - end - end - end - end end diff --git a/spec_legacy/unit/query_spec.rb b/spec_legacy/unit/query_spec.rb index 388eb07e6e..aea6c2e6b3 100644 --- a/spec_legacy/unit/query_spec.rb +++ b/spec_legacy/unit/query_spec.rb @@ -258,27 +258,6 @@ describe Query, type: :model do assert q.has_column?(c) end - it 'should groupable columns should include custom fields' do - q = Query.new name: '_' - assert q.groupable_columns.detect { |c| c.is_a? Queries::WorkPackages::Columns::CustomFieldColumn } - end - - it 'should grouped with valid column' do - q = Query.new(group_by: 'status', name: '_') - assert q.grouped? - refute_nil q.group_by_column - assert_equal :status, q.group_by_column.name - refute_nil q.group_by_statement - assert_equal 'status', q.group_by_statement - end - - it 'should grouped with invalid column' do - q = Query.new(group_by: 'foo', name: '_') - assert !q.grouped? - assert_nil q.group_by_column - assert_nil q.group_by_statement - end - it 'should default sort' do q = Query.new name: '_' assert_equal [], q.sort_criteria @@ -347,50 +326,6 @@ describe Query, type: :model do assert_equal values.sort, values end - it 'should invalid query should raise query statement invalid error' do - q = Query.new name: '_' - assert_raises ActiveRecord::StatementInvalid do - q.results(conditions: 'foo = 1').work_packages.to_a - end - end - - it 'should issue count by association group' do - q = Query.new(name: '_', - group_by: 'assigned_to', - show_hierarchies: false) - - count_by_group = q.results.work_package_count_by_group - assert_kind_of Hash, count_by_group - assert_equal %w(NilClass User), count_by_group.keys.map { |k| k.class.name }.uniq.sort - assert_equal %w(Integer), count_by_group.values.map { |k| k.class.name }.uniq - assert count_by_group.has_key?(User.find(3)) - end - - it 'should issue count by list custom field group' do - q = Query.new(name: '_', - group_by: 'cf_1', - show_hierarchies: false) - - count_by_group = q.results.work_package_count_by_group - assert_kind_of Hash, count_by_group - expect(count_by_group.keys.map { |k| k.class.name }.uniq) - .to match_array(%w(CustomOption NilClass)) - assert_equal %w(Integer), count_by_group.values.map { |k| k.class.name }.uniq - expect(count_by_group.any? { |k, v| k.is_a?(CustomOption) && k.id == 1 && v == 1 }) - .to be_truthy - end - - it 'should issue count by date custom field group' do - q = Query.new(name: '_', - group_by: 'cf_8', - show_hierarchies: false) - - count_by_group = q.results.work_package_count_by_group - assert_kind_of Hash, count_by_group - assert_equal %w(Date NilClass), count_by_group.keys.map { |k| k.class.name }.uniq.sort - assert_equal %w(Integer), count_by_group.values.map { |k| k.class.name }.uniq - end - context '#filter_for' do before do @query = Query.new(name: '_')