Merge pull request #7664 from opf/feature/sorted_wp_group_counts

Feature/sorted wp group counts

[ci skip]

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

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

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
 

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

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

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: '_')