From 72dcb214bb62e11317313aca16e33a5bba0dd3b9 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 27 Sep 2024 13:47:42 +0200 Subject: [PATCH 01/28] [#57305] Sorting by custom field has strong impact on performance for the project list https://community.openproject.org/work_packages/57305 From b6f64e2cfa078d1f55bb6627ea7e7812cf982ce2 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 27 Sep 2024 13:51:03 +0200 Subject: [PATCH 02/28] remove unused option `if` from work package property select --- .../queries/work_packages/selects/property_select.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/app/models/queries/work_packages/selects/property_select.rb b/app/models/queries/work_packages/selects/property_select.rb index c73db63aaeff..0a7da338fe3b 100644 --- a/app/models/queries/work_packages/selects/property_select.rb +++ b/app/models/queries/work_packages/selects/property_select.rb @@ -130,15 +130,12 @@ def caption shared_with_users: { sortable: false, groupable: false - } } def self.instances(_context = nil) - property_selects.filter_map do |name, options| - next unless !options[:if] || options[:if].call - - new(name, options.except(:if)) + property_selects.map do |name, options| + new(name, options) end end end From 4f77de64f520b4cac970d694976b9bfc439e905a Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 27 Sep 2024 15:36:23 +0200 Subject: [PATCH 03/28] using instance variables instead of writers for work package selects --- .../selects/custom_field_select.rb | 9 +-- .../selects/relation_of_type_select.rb | 7 +- .../work_packages/selects/relation_select.rb | 2 +- .../selects/relation_to_type_select.rb | 9 +-- .../selects/work_package_select.rb | 78 ++++++++----------- .../selects/shared_query_select_specs.rb | 32 ++++---- 6 files changed, 57 insertions(+), 80 deletions(-) diff --git a/app/models/queries/work_packages/selects/custom_field_select.rb b/app/models/queries/work_packages/selects/custom_field_select.rb index c6e2a107d5da..21a0f0c730bb 100644 --- a/app/models/queries/work_packages/selects/custom_field_select.rb +++ b/app/models/queries/work_packages/selects/custom_field_select.rb @@ -69,20 +69,19 @@ def self.instances(context = nil) private def set_name! - self.name = custom_field.column_name.to_sym + @name = custom_field.column_name.to_sym end def set_sortable! - self.sortable = custom_field.order_statements || false + @sortable = custom_field.order_statements || false end def set_groupable! - self.groupable = custom_field.group_by_statement if groupable_custom_field?(custom_field) - self.groupable ||= false + @groupable = groupable_custom_field?(custom_field) ? custom_field.group_by_statement || false : false end def set_summable! - self.summable = if %w(float int).include?(custom_field.field_format) + @summable = if %w(float int).include?(custom_field.field_format) select = summable_select_statement ->(query, grouped) { diff --git a/app/models/queries/work_packages/selects/relation_of_type_select.rb b/app/models/queries/work_packages/selects/relation_of_type_select.rb index eb6809cec69c..f7dd9d4d668e 100644 --- a/app/models/queries/work_packages/selects/relation_of_type_select.rb +++ b/app/models/queries/work_packages/selects/relation_of_type_select.rb @@ -28,12 +28,9 @@ class Queries::WorkPackages::Selects::RelationOfTypeSelect < Queries::WorkPackages::Selects::RelationSelect def initialize(type) - self.type = type - super(name) - end + super(:"relations_of_type_#{type[:sym]}") - def name - :"relations_of_type_#{type[:sym]}" + @type = type end def sym diff --git a/app/models/queries/work_packages/selects/relation_select.rb b/app/models/queries/work_packages/selects/relation_select.rb index c12f7663eb2e..224b1e58f743 100644 --- a/app/models/queries/work_packages/selects/relation_select.rb +++ b/app/models/queries/work_packages/selects/relation_select.rb @@ -27,7 +27,7 @@ #++ class Queries::WorkPackages::Selects::RelationSelect < Queries::WorkPackages::Selects::WorkPackageSelect - attr_accessor :type + attr_reader :type def self.granted_by_enterprise_token EnterpriseToken.allows_to?(:work_package_query_relation_columns) diff --git a/app/models/queries/work_packages/selects/relation_to_type_select.rb b/app/models/queries/work_packages/selects/relation_to_type_select.rb index cd6ddcd432fc..7744ed717f10 100644 --- a/app/models/queries/work_packages/selects/relation_to_type_select.rb +++ b/app/models/queries/work_packages/selects/relation_to_type_select.rb @@ -28,14 +28,9 @@ class Queries::WorkPackages::Selects::RelationToTypeSelect < Queries::WorkPackages::Selects::RelationSelect def initialize(type) - super + super(:"relations_to_type_#{type.id}") - set_name! type - self.type = type - end - - def set_name!(type) - self.name = :"relations_to_type_#{type.id}" + @type = type end def caption diff --git a/app/models/queries/work_packages/selects/work_package_select.rb b/app/models/queries/work_packages/selects/work_package_select.rb index a3ffe42aa3b6..af559df44bd1 100644 --- a/app/models/queries/work_packages/selects/work_package_select.rb +++ b/app/models/queries/work_packages/selects/work_package_select.rb @@ -27,19 +27,13 @@ #++ class Queries::WorkPackages::Selects::WorkPackageSelect - attr_accessor :highlightable, - :name, - :sortable_join, - :summable, - :default_order, - :association - alias_method :highlightable?, :highlightable - - attr_reader :groupable, - :sortable, - :displayable - - attr_writer :null_handling, + attr_reader :highlightable, + :name, + :sortable_join, + :summable, + :default_order, + :association, + :null_handling, :summable_select, :summable_work_packages_select @@ -76,31 +70,25 @@ def null_handling(_asc) @null_handling end - def groupable=(value) - @groupable = name_or_value_or_false(value) + def displayable + @displayable.nil? ? true : @displayable end - def sortable=(value) - @sortable = name_or_value_or_false(value) + def sortable + name_or_value_or_false(@sortable) end - def displayable=(value) - @displayable = value.nil? ? true : value + def groupable + name_or_value_or_false(@groupable) end - def displayable? - displayable - end + def displayable? = !!displayable - # Returns true if the column is sortable, otherwise false - def sortable? - !!sortable - end + def sortable? = !!sortable - # Returns true if the column is groupable, otherwise false - def groupable? - !!groupable - end + def groupable? = !!groupable + + def highlightable? = !!highlightable def summable? summable || @summable_select || @summable_work_packages_select @@ -127,22 +115,24 @@ def value(model) end def initialize(name, options = {}) - self.name = name - - %i(sortable - sortable_join - displayable - groupable - summable - summable_select - summable_work_packages_select - association - null_handling - default_order).each do |attribute| - send(:"#{attribute}=", options[attribute]) + @name = name + + %i[ + sortable + sortable_join + displayable + groupable + summable + summable_select + summable_work_packages_select + association + null_handling + default_order + ].each do |attribute| + instance_variable_set(:"@#{attribute}", options[attribute]) end - self.highlightable = !!options.fetch(:highlightable, false) + @highlightable = !!options.fetch(:highlightable, false) end def caption diff --git a/spec/models/queries/work_packages/selects/shared_query_select_specs.rb b/spec/models/queries/work_packages/selects/shared_query_select_specs.rb index 4fd9758d4c28..8df7f50fddc2 100644 --- a/spec/models/queries/work_packages/selects/shared_query_select_specs.rb +++ b/spec/models/queries/work_packages/selects/shared_query_select_specs.rb @@ -29,25 +29,25 @@ RSpec.shared_examples_for "query column" do |sortable_by_default: false| describe "#groupable" do it "is the name if true is provided" do - instance.groupable = true + instance.instance_variable_set(:@groupable, true) expect(instance.groupable).to eql(instance.name.to_s) end it "is the value if something truthy is provided" do - instance.groupable = "lorem ipsum" + instance.instance_variable_set(:@groupable, "lorem ipsum") expect(instance.groupable).to eql("lorem ipsum") end it "is false if false is provided" do - instance.groupable = false + instance.instance_variable_set(:@groupable, false) expect(instance.groupable).to be_falsey end - it "is false if nothing is provided" do - instance.groupable = nil + it "is false if nil is provided" do + instance.instance_variable_set(:@groupable, nil) expect(instance.groupable).to be_falsey end @@ -55,43 +55,39 @@ describe "#sortable" do it "is the name if true is provided" do - instance.sortable = true + instance.instance_variable_set(:@sortable, true) expect(instance.sortable).to eql(instance.name.to_s) end it "is the value if something truthy is provided" do - instance.sortable = "lorem ipsum" + instance.instance_variable_set(:@sortable, "lorem ipsum") expect(instance.sortable).to eql("lorem ipsum") end it "is false if false is provided" do - instance.sortable = false + instance.instance_variable_set(:@sortable, false) expect(instance.sortable).to be_falsey end - it "is false if nothing is provided" do - instance.sortable = nil + it "is false if nil is provided" do + instance.instance_variable_set(:@sortable, nil) expect(instance.sortable).to be_falsey end end describe "#groupable?" do - it "is false by default" do - expect(instance).not_to be_groupable - end - it "is true if told so" do - instance.groupable = true + instance.instance_variable_set(:@groupable, true) expect(instance).to be_groupable end it "is true if a value is provided (e.g. for specifying sql code)" do - instance.groupable = "COALESCE(null, 1)" + instance.instance_variable_set(:@groupable, "COALESCE(null, 1)") expect(instance).to be_groupable end @@ -107,13 +103,13 @@ end it "is true if told so" do - instance.sortable = true + instance.instance_variable_set(:@sortable, true) expect(instance).to be_sortable end it "is true if a value is provided (e.g. for specifying sql code)" do - instance.sortable = "COALESCE(null, 1)" + instance.instance_variable_set(:@sortable, "COALESCE(null, 1)") expect(instance).to be_sortable end From 82a7e2ef0602f5bebcfe05506745b69126feb6cb Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 4 Oct 2024 15:04:46 +0200 Subject: [PATCH 04/28] inline set_xxx! methods in CustomFieldSelect --- .../selects/custom_field_select.rb | 46 +++++++------------ 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/app/models/queries/work_packages/selects/custom_field_select.rb b/app/models/queries/work_packages/selects/custom_field_select.rb index 21a0f0c730bb..38e429839adf 100644 --- a/app/models/queries/work_packages/selects/custom_field_select.rb +++ b/app/models/queries/work_packages/selects/custom_field_select.rb @@ -32,10 +32,10 @@ def initialize(custom_field) @cf = custom_field - set_name! - set_sortable! - set_groupable! - set_summable! + @name = custom_field.column_name.to_sym + @sortable = custom_field.order_statements || false + @groupable = groupable_custom_field?(custom_field) ? custom_field.group_by_statement || false : false + @summable = summable_statement end def groupable_custom_field?(custom_field) @@ -68,31 +68,6 @@ def self.instances(context = nil) private - def set_name! - @name = custom_field.column_name.to_sym - end - - def set_sortable! - @sortable = custom_field.order_statements || false - end - - def set_groupable! - @groupable = groupable_custom_field?(custom_field) ? custom_field.group_by_statement || false : false - end - - def set_summable! - @summable = if %w(float int).include?(custom_field.field_format) - select = summable_select_statement - - ->(query, grouped) { - Queries::WorkPackages::Selects::WorkPackageSelect - .scoped_column_sum(summable_scope(query), select, grouped && query.group_by_statement) - } - else - false - end - end - def summable_scope(query) WorkPackage .where(id: query.results.work_packages) @@ -109,4 +84,17 @@ def summable_select_statement "COALESCE(ROUND(SUM(value::NUMERIC), 2)::FLOAT, 0.0) #{name}" end end + + def summable_statement + if %w[float int].include?(custom_field.field_format) + select = summable_select_statement + + ->(query, grouped) { + Queries::WorkPackages::Selects::WorkPackageSelect + .scoped_column_sum(summable_scope(query), select, grouped && query.group_by_statement) + } + else + false + end + end end From 86ab142d89c5c762dea7beaf908e1b54c3642281 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 4 Oct 2024 14:56:26 +0200 Subject: [PATCH 05/28] rename order_statements to order_statement swithcing it to return single statement --- app/models/custom_field/order_statements.rb | 14 +++++++------- .../queries/projects/orders/custom_field_order.rb | 4 +--- .../work_packages/selects/custom_field_select.rb | 2 +- app/models/query/results.rb | 4 +--- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 98c46c8c8167..79b773ec7db2 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -30,21 +30,21 @@ module CustomField::OrderStatements # Returns a ORDER BY clause that can used to sort customized # objects by their value of the custom field. # Returns false, if the custom field can not be used for sorting. - def order_statements + def order_statement case field_format when "list" - [order_by_list_sql] + order_by_list_sql when "string", "date", "bool", "link" - [coalesce_select_custom_value_as_string] + coalesce_select_custom_value_as_string when "int", "float" # Make the database cast values into numeric # Postgresql will raise an error if a value can not be casted! # CustomValue validations should ensure that it doesn't occur - [select_custom_value_as_decimal] + select_custom_value_as_decimal when "user" - [order_by_user_sql] + order_by_user_sql when "version" - [order_by_version_sql] + order_by_version_sql end end @@ -61,7 +61,7 @@ def null_handling(asc) # which differ for multi-value select fields, # because in this case we do want the primary CV values def group_by_statement - return order_statements unless field_format == "list" + return order_statement unless field_format == "list" if multi_value? # We want to return the internal IDs in the case of grouping diff --git a/app/models/queries/projects/orders/custom_field_order.rb b/app/models/queries/projects/orders/custom_field_order.rb index 30aa9e3b4558..d9ca8de9d1c3 100644 --- a/app/models/queries/projects/orders/custom_field_order.rb +++ b/app/models/queries/projects/orders/custom_field_order.rb @@ -58,9 +58,7 @@ def available? private def order(scope) - joined_statement = custom_field.order_statements.map do |statement| - Arel.sql("#{statement} #{direction}") - end + joined_statement = Arel.sql("#{custom_field.order_statement} #{direction}") scope.order(joined_statement) end diff --git a/app/models/queries/work_packages/selects/custom_field_select.rb b/app/models/queries/work_packages/selects/custom_field_select.rb index 38e429839adf..e4b09ad81b9c 100644 --- a/app/models/queries/work_packages/selects/custom_field_select.rb +++ b/app/models/queries/work_packages/selects/custom_field_select.rb @@ -33,7 +33,7 @@ def initialize(custom_field) @cf = custom_field @name = custom_field.column_name.to_sym - @sortable = custom_field.order_statements || false + @sortable = custom_field.order_statement || false @groupable = groupable_custom_field?(custom_field) ? custom_field.group_by_statement || false : false @summable = summable_statement end diff --git a/app/models/query/results.rb b/app/models/query/results.rb index dbf58dcbfa8c..51a099545805 100644 --- a/app/models/query/results.rb +++ b/app/models/query/results.rb @@ -195,10 +195,8 @@ def columns_hash_for(association = nil) ## # Return the case insensitive version for columns with a string type def case_insensitive_condition(column_key, condition, columns_hash) - if columns_hash[column_key]&.type == :string + if columns_hash[column_key]&.type == :string || custom_field_type(column_key) == "string" "LOWER(#{condition})" - elsif custom_field_type(column_key) == "string" - condition.map { |c| "LOWER(#{c})" } else condition end From ae8d82aa99e08dbbd49c9451318d679eb0b620d1 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 4 Oct 2024 16:35:31 +0200 Subject: [PATCH 06/28] apply null handling to ordering projects by custom field --- app/models/queries/projects/orders/custom_field_order.rb | 8 ++++++-- .../projects/project_query_custom_field_order_spec.rb | 8 ++++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/app/models/queries/projects/orders/custom_field_order.rb b/app/models/queries/projects/orders/custom_field_order.rb index d9ca8de9d1c3..5b3f235acfcf 100644 --- a/app/models/queries/projects/orders/custom_field_order.rb +++ b/app/models/queries/projects/orders/custom_field_order.rb @@ -58,8 +58,12 @@ def available? private def order(scope) - joined_statement = Arel.sql("#{custom_field.order_statement} #{direction}") + order_statement = "#{custom_field.order_statement} #{direction}" - scope.order(joined_statement) + if (null_handling = custom_field.null_handling(direction == :asc)) + order_statement = "#{order_statement} #{null_handling}" + end + + scope.order(Arel.sql(order_statement)) end end diff --git a/spec/models/queries/projects/project_query_custom_field_order_spec.rb b/spec/models/queries/projects/project_query_custom_field_order_spec.rb index 5958575f31b1..c430470ea6fe 100644 --- a/spec/models/queries/projects/project_query_custom_field_order_spec.rb +++ b/spec/models/queries/projects/project_query_custom_field_order_spec.rb @@ -116,9 +116,9 @@ def project_without_cf_value let(:projects) do [ + project_without_cf_value, project_with_cf_value("6"), - project_with_cf_value("16"), - project_without_cf_value # TODO: should be at index 0 + project_with_cf_value("16") ] end end @@ -130,9 +130,9 @@ def project_without_cf_value let(:projects) do [ + project_without_cf_value, project_with_cf_value("6.25"), - project_with_cf_value("16"), - project_without_cf_value # TODO: should be at index 0 + project_with_cf_value("16") ] end end From 544e7d4b8ce78d327f106369c7141607ad267b1d Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 4 Oct 2024 17:16:04 +0200 Subject: [PATCH 07/28] rename CustomField#null_handling to order_null_handling --- app/models/custom_field/order_statements.rb | 6 +++--- app/models/queries/projects/orders/custom_field_order.rb | 2 +- .../queries/work_packages/selects/custom_field_select.rb | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 79b773ec7db2..62484202945d 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -48,9 +48,9 @@ def order_statement end end - ## - # Returns the null handling for the given direction - def null_handling(asc) + # Returns the ORDER BY option defining order of objects without value for the + # custom field. + def order_null_handling(asc) return unless %w[int float].include?(field_format) null_direction = asc ? "FIRST" : "LAST" diff --git a/app/models/queries/projects/orders/custom_field_order.rb b/app/models/queries/projects/orders/custom_field_order.rb index 5b3f235acfcf..159d38034485 100644 --- a/app/models/queries/projects/orders/custom_field_order.rb +++ b/app/models/queries/projects/orders/custom_field_order.rb @@ -60,7 +60,7 @@ def available? def order(scope) order_statement = "#{custom_field.order_statement} #{direction}" - if (null_handling = custom_field.null_handling(direction == :asc)) + if (null_handling = custom_field.order_null_handling(direction == :asc)) order_statement = "#{order_statement} #{null_handling}" end diff --git a/app/models/queries/work_packages/selects/custom_field_select.rb b/app/models/queries/work_packages/selects/custom_field_select.rb index e4b09ad81b9c..45953f7ca380 100644 --- a/app/models/queries/work_packages/selects/custom_field_select.rb +++ b/app/models/queries/work_packages/selects/custom_field_select.rb @@ -46,7 +46,7 @@ def caption @cf.name end - delegate :null_handling, to: :custom_field + def null_handling(...) = custom_field.order_null_handling(...) def custom_field @cf From edd62d8bf9cf18f11ed9ceaaad4047ac938e2b33 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 4 Oct 2024 20:36:11 +0200 Subject: [PATCH 08/28] using joins for custom field ordering, grouping should be broken --- app/models/custom_field/order_statements.rb | 247 ++++++++++++------ .../projects/orders/custom_field_order.rb | 4 + .../selects/custom_field_select.rb | 1 + 3 files changed, 175 insertions(+), 77 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 62484202945d..435852ecb15e 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -27,31 +27,38 @@ #++ module CustomField::OrderStatements - # Returns a ORDER BY clause that can used to sort customized - # objects by their value of the custom field. - # Returns false, if the custom field can not be used for sorting. + # Returns the expression to use in ORDER BY clause to sort objects by their + # value of the custom field. def order_statement case field_format - when "list" - order_by_list_sql + when "string", "date", "bool", "link", "int", "float", "list", "user", "version" + "cf_order_#{id}.value" + end + end + + # Returns the join statement that is required to sort objects by their value + # of the custom field. + def order_join_statement + case field_format when "string", "date", "bool", "link" - coalesce_select_custom_value_as_string - when "int", "float" - # Make the database cast values into numeric - # Postgresql will raise an error if a value can not be casted! - # CustomValue validations should ensure that it doesn't occur - select_custom_value_as_decimal + join_for_order_by_string_sql + when "int" + join_for_order_by_int_sql + when "float" + join_for_order_by_float_sql + when "list" + join_for_order_by_list_sql when "user" - order_by_user_sql + join_for_order_by_user_sql when "version" - order_by_version_sql + join_for_order_by_version_sql end end # Returns the ORDER BY option defining order of objects without value for the # custom field. def order_null_handling(asc) - return unless %w[int float].include?(field_format) + return unless %w[int float string date bool link].include?(field_format) null_direction = asc ? "FIRST" : "LAST" Arel.sql("NULLS #{null_direction}") @@ -73,6 +80,155 @@ def group_by_statement private + def join_for_order_by_string_sql + <<-SQL.squish + LEFT OUTER JOIN ( + SELECT DISTINCT ON (cv.customized_id) cv.customized_id, cv.value "value" + FROM #{CustomValue.quoted_table_name} cv + WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} + AND cv.custom_field_id = #{id} + AND cv.value IS NOT NULL + AND cv.value != '' + ORDER BY cv.customized_id, cv.id + ) cf_order_#{id} + ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id + SQL + end + + def join_for_order_by_int_sql + <<-SQL.squish + LEFT OUTER JOIN ( + SELECT DISTINCT ON (cv.customized_id) cv.customized_id, cv.value::decimal(60) "value" + FROM #{CustomValue.quoted_table_name} cv + WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} + AND cv.custom_field_id = #{id} + AND cv.value IS NOT NULL + AND cv.value != '' + ORDER BY cv.customized_id, cv.id + ) cf_order_#{id} + ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id + SQL + end + + def join_for_order_by_float_sql + <<-SQL.squish + LEFT OUTER JOIN ( + SELECT DISTINCT ON (cv.customized_id) cv.customized_id, cv.value::double precision "value" + FROM #{CustomValue.quoted_table_name} cv + WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} + AND cv.custom_field_id = #{id} + AND cv.value IS NOT NULL + AND cv.value != '' + ORDER BY cv.customized_id, cv.id + ) cf_order_#{id} + ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id + SQL + end + + def join_for_order_by_list_sql + if multi_value? + <<-SQL.squish + LEFT OUTER JOIN ( + SELECT cv.customized_id, array_agg(co.position ORDER BY co.position) "value" + FROM #{CustomValue.quoted_table_name} cv + INNER JOIN #{CustomOption.quoted_table_name} co + ON co.id = cv.value::bigint + WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} + AND cv.custom_field_id = #{id} + AND cv.value IS NOT NULL + AND cv.value != '' + GROUP BY cv.customized_id + ) cf_order_#{id} + ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id + SQL + else + <<-SQL.squish + LEFT OUTER JOIN ( + SELECT DISTINCT ON (cv.customized_id) cv.customized_id, co.position "value" + FROM #{CustomValue.quoted_table_name} cv + INNER JOIN #{CustomOption.quoted_table_name} co + ON co.id = cv.value::bigint + WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} + AND cv.custom_field_id = #{id} + AND cv.value IS NOT NULL + AND cv.value != '' + ORDER BY cv.customized_id, cv.id + ) cf_order_#{id} + ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id + SQL + end + end + + def join_for_order_by_user_sql + columns_array = "ARRAY[users.lastname, users.firstname, users.mail]" + + if multi_value? + <<-SQL.squish + LEFT OUTER JOIN ( + SELECT cv.customized_id, ARRAY_AGG(#{columns_array} ORDER BY #{columns_array}) "value" + FROM #{CustomValue.quoted_table_name} cv + INNER JOIN #{User.quoted_table_name} users + ON users.id = cv.value::bigint + WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} + AND cv.custom_field_id = #{id} + AND cv.value IS NOT NULL + AND cv.value != '' + GROUP BY cv.customized_id + ) cf_order_#{id} + ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id + SQL + else + <<-SQL.squish + LEFT OUTER JOIN ( + SELECT DISTINCT ON (cv.customized_id) cv.customized_id, #{columns_array} "value" + FROM #{CustomValue.quoted_table_name} cv + INNER JOIN #{User.quoted_table_name} users + ON users.id = cv.value::bigint + WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} + AND cv.custom_field_id = #{id} + AND cv.value IS NOT NULL + AND cv.value != '' + ORDER BY cv.customized_id, cv.id + ) cf_order_#{id} + ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id + SQL + end + end + + def join_for_order_by_version_sql + if multi_value? + <<-SQL.squish + LEFT OUTER JOIN ( + SELECT cv.customized_id, array_agg(versions.name ORDER BY versions.name) "value" + FROM #{CustomValue.quoted_table_name} cv + INNER JOIN #{Version.quoted_table_name} versions + ON versions.id = cv.value::bigint + WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} + AND cv.custom_field_id = #{id} + AND cv.value IS NOT NULL + AND cv.value != '' + GROUP BY cv.customized_id + ) cf_order_#{id} + ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id + SQL + else + <<-SQL.squish + LEFT OUTER JOIN ( + SELECT DISTINCT ON (cv.customized_id) cv.customized_id, versions.name "value" + FROM #{CustomValue.quoted_table_name} cv + INNER JOIN #{Version.quoted_table_name} versions + ON versions.id = cv.value::bigint + WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} + AND cv.custom_field_id = #{id} + AND cv.value IS NOT NULL + AND cv.value != '' + ORDER BY cv.customized_id, cv.id + ) cf_order_#{id} + ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id + SQL + end + end + def coalesce_select_custom_value_as_string # COALESCE is here to make sure that blank and NULL values are sorted equally <<-SQL.squish @@ -105,69 +261,6 @@ def select_custom_values_as_group SQL end - def select_custom_value_as_decimal - <<-SQL.squish - ( - SELECT CAST(cv_sort.value AS decimal(60,3)) - FROM #{CustomValue.quoted_table_name} cv_sort - WHERE #{cv_sort_only_custom_field_condition_sql} - AND cv_sort.value <> '' - AND cv_sort.value IS NOT NULL - LIMIT 1 - ) - SQL - end - - def order_by_list_sql - columns = multi_value? ? "array_agg(co_sort.position ORDER BY co_sort.position)" : "co_sort.position" - limit = multi_value? ? "" : "LIMIT 1" - - <<-SQL.squish - ( - SELECT #{columns} - FROM #{CustomOption.quoted_table_name} co_sort - INNER JOIN #{CustomValue.quoted_table_name} cv_sort - ON cv_sort.value IS NOT NULL AND cv_sort.value != '' AND co_sort.id = cv_sort.value::bigint - WHERE #{cv_sort_only_custom_field_condition_sql} - #{limit} - ) - SQL - end - - def order_by_user_sql - columns_array = "ARRAY[cv_user.lastname, cv_user.firstname, cv_user.mail]" - - columns = multi_value? ? "array_agg(#{columns_array} ORDER BY #{columns_array})" : columns_array - limit = multi_value? ? "" : "LIMIT 1" - - <<-SQL.squish - ( - SELECT #{columns} - FROM #{User.quoted_table_name} cv_user - INNER JOIN #{CustomValue.quoted_table_name} cv_sort - ON cv_sort.value IS NOT NULL AND cv_sort.value != '' AND cv_user.id = cv_sort.value::bigint - WHERE #{cv_sort_only_custom_field_condition_sql} - #{limit} - ) - SQL - end - - def order_by_version_sql - columns = multi_value? ? "array_agg(cv_version.name ORDER BY cv_version.name)" : "cv_version.name" - limit = multi_value? ? "" : "LIMIT 1" - - <<-SQL.squish - ( - SELECT #{columns} - FROM #{Version.quoted_table_name} cv_version - INNER JOIN #{CustomValue.quoted_table_name} cv_sort - ON cv_sort.value IS NOT NULL AND cv_sort.value != '' AND cv_version.id = cv_sort.value::bigint - WHERE #{cv_sort_only_custom_field_condition_sql} - #{limit} - ) - SQL - end - def cv_sort_only_custom_field_condition_sql <<-SQL.squish cv_sort.customized_type='#{self.class.customized_class.name}' diff --git a/app/models/queries/projects/orders/custom_field_order.rb b/app/models/queries/projects/orders/custom_field_order.rb index 159d38034485..b7ad4e3cf9f7 100644 --- a/app/models/queries/projects/orders/custom_field_order.rb +++ b/app/models/queries/projects/orders/custom_field_order.rb @@ -58,6 +58,10 @@ def available? private def order(scope) + if (join_statement = custom_field.order_join_statement) + scope = scope.joins(join_statement) + end + order_statement = "#{custom_field.order_statement} #{direction}" if (null_handling = custom_field.order_null_handling(direction == :asc)) diff --git a/app/models/queries/work_packages/selects/custom_field_select.rb b/app/models/queries/work_packages/selects/custom_field_select.rb index 45953f7ca380..b1c089ff8253 100644 --- a/app/models/queries/work_packages/selects/custom_field_select.rb +++ b/app/models/queries/work_packages/selects/custom_field_select.rb @@ -34,6 +34,7 @@ def initialize(custom_field) @name = custom_field.column_name.to_sym @sortable = custom_field.order_statement || false + @sortable_join = custom_field.order_join_statement || false @groupable = groupable_custom_field?(custom_field) ? custom_field.group_by_statement || false : false @summable = summable_statement end From b886078e28f8e473d41828351ed99ab5defdf0db Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 4 Oct 2024 15:19:29 +0200 Subject: [PATCH 09/28] move deciding which custom field can be grouped by to order statements --- app/models/custom_field/order_statements.rb | 2 ++ .../queries/work_packages/selects/custom_field_select.rb | 6 +----- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 435852ecb15e..ef4da8fe948e 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -68,6 +68,8 @@ def order_null_handling(asc) # which differ for multi-value select fields, # because in this case we do want the primary CV values def group_by_statement + return unless field_format.in?(%w[list date bool int]) + return order_statement unless field_format == "list" if multi_value? diff --git a/app/models/queries/work_packages/selects/custom_field_select.rb b/app/models/queries/work_packages/selects/custom_field_select.rb index b1c089ff8253..5c6931cd6e40 100644 --- a/app/models/queries/work_packages/selects/custom_field_select.rb +++ b/app/models/queries/work_packages/selects/custom_field_select.rb @@ -35,14 +35,10 @@ def initialize(custom_field) @name = custom_field.column_name.to_sym @sortable = custom_field.order_statement || false @sortable_join = custom_field.order_join_statement || false - @groupable = groupable_custom_field?(custom_field) ? custom_field.group_by_statement || false : false + @groupable = custom_field.group_by_statement || false @summable = summable_statement end - def groupable_custom_field?(custom_field) - %w(list date bool int).include?(custom_field.field_format) - end - def caption @cf.name end From 4a72b89d3ed494ff39283d5bf806d52e6e1a5255 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 10 Oct 2024 19:25:24 +0200 Subject: [PATCH 10/28] enable grouping by float, string and link too --- app/models/custom_field/order_statements.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index ef4da8fe948e..bcf700e823a8 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -68,7 +68,7 @@ def order_null_handling(asc) # which differ for multi-value select fields, # because in this case we do want the primary CV values def group_by_statement - return unless field_format.in?(%w[list date bool int]) + return unless field_format.in?(%w[list date bool int float string link]) return order_statement unless field_format == "list" From ac274830c7c333a0a9186e3b735174ae54f5ee4d Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 10 Oct 2024 19:47:31 +0200 Subject: [PATCH 11/28] remove replacing nil by false which doesn't seem to be needed --- .../queries/work_packages/selects/custom_field_select.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/queries/work_packages/selects/custom_field_select.rb b/app/models/queries/work_packages/selects/custom_field_select.rb index 5c6931cd6e40..e24e2825f1bc 100644 --- a/app/models/queries/work_packages/selects/custom_field_select.rb +++ b/app/models/queries/work_packages/selects/custom_field_select.rb @@ -33,9 +33,9 @@ def initialize(custom_field) @cf = custom_field @name = custom_field.column_name.to_sym - @sortable = custom_field.order_statement || false - @sortable_join = custom_field.order_join_statement || false - @groupable = custom_field.group_by_statement || false + @sortable = custom_field.order_statement + @sortable_join = custom_field.order_join_statement + @groupable = custom_field.group_by_statement @summable = summable_statement end From 48eda6efa58ed53f41f6312d8221ca7557745246 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Tue, 8 Oct 2024 19:44:51 +0200 Subject: [PATCH 12/28] handle joins for groupping by custom field --- app/models/custom_field/order_statements.rb | 58 +++---------------- .../selects/custom_field_select.rb | 1 + .../selects/work_package_select.rb | 2 + app/models/query.rb | 6 +- app/models/query/results.rb | 1 + app/models/query/results/group_by.rb | 1 + app/models/query/results/sums.rb | 2 +- 7 files changed, 19 insertions(+), 52 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index bcf700e823a8..25401cb60cbc 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -64,20 +64,18 @@ def order_null_handling(asc) Arel.sql("NULLS #{null_direction}") end - # Returns the grouping result - # which differ for multi-value select fields, - # because in this case we do want the primary CV values + # Returns the expression to use in GROUP BY (and ORDER BY) clause to group + # objects by their value of the custom field. def group_by_statement return unless field_format.in?(%w[list date bool int float string link]) - return order_statement unless field_format == "list" + order_statement + end - if multi_value? - # We want to return the internal IDs in the case of grouping - select_custom_values_as_group - else - coalesce_select_custom_value_as_string - end + # Returns the join statement that is required to group objects by their value + # of the custom field. + def group_by_join_statement + order_join_statement end private @@ -230,44 +228,4 @@ def join_for_order_by_version_sql SQL end end - - def coalesce_select_custom_value_as_string - # COALESCE is here to make sure that blank and NULL values are sorted equally - <<-SQL.squish - COALESCE(#{select_custom_value_as_string}, '') - SQL - end - - def select_custom_value_as_string - <<-SQL.squish - ( - SELECT cv_sort.value - FROM #{CustomValue.quoted_table_name} cv_sort - WHERE #{cv_sort_only_custom_field_condition_sql} - LIMIT 1 - ) - SQL - end - - def select_custom_values_as_group - <<-SQL.squish - COALESCE( - ( - SELECT string_agg(cv_sort.value, '.') - FROM #{CustomValue.quoted_table_name} cv_sort - WHERE #{cv_sort_only_custom_field_condition_sql} - AND cv_sort.value IS NOT NULL - ), - '' - ) - SQL - end - - def cv_sort_only_custom_field_condition_sql - <<-SQL.squish - cv_sort.customized_type='#{self.class.customized_class.name}' - AND cv_sort.customized_id=#{self.class.customized_class.quoted_table_name}.id - AND cv_sort.custom_field_id=#{id} - SQL - end end diff --git a/app/models/queries/work_packages/selects/custom_field_select.rb b/app/models/queries/work_packages/selects/custom_field_select.rb index e24e2825f1bc..584dbfb44be9 100644 --- a/app/models/queries/work_packages/selects/custom_field_select.rb +++ b/app/models/queries/work_packages/selects/custom_field_select.rb @@ -36,6 +36,7 @@ def initialize(custom_field) @sortable = custom_field.order_statement @sortable_join = custom_field.order_join_statement @groupable = custom_field.group_by_statement + @groupable_join = custom_field.group_by_join_statement @summable = summable_statement end diff --git a/app/models/queries/work_packages/selects/work_package_select.rb b/app/models/queries/work_packages/selects/work_package_select.rb index af559df44bd1..947215515127 100644 --- a/app/models/queries/work_packages/selects/work_package_select.rb +++ b/app/models/queries/work_packages/selects/work_package_select.rb @@ -30,6 +30,7 @@ class Queries::WorkPackages::Selects::WorkPackageSelect attr_reader :highlightable, :name, :sortable_join, + :groupable_join, :summable, :default_order, :association, @@ -122,6 +123,7 @@ def initialize(name, options = {}) sortable_join displayable groupable + groupable_join summable summable_select summable_work_packages_select diff --git a/app/models/query.rb b/app/models/query.rb index d8051b6b1d06..cd071156a38c 100644 --- a/app/models/query.rb +++ b/app/models/query.rb @@ -359,7 +359,11 @@ def group_by_column end def group_by_statement - group_by_column.try(:groupable) + group_by_column&.groupable + end + + def group_by_join_statement + group_by_column&.groupable_join end def statement diff --git a/app/models/query/results.rb b/app/models/query/results.rb index 51a099545805..8bffcccd8ad0 100644 --- a/app/models/query/results.rb +++ b/app/models/query/results.rb @@ -82,6 +82,7 @@ def filtered_work_packages def sorted_work_packages work_package_scope .joins(sort_criteria_joins) + .joins(query.group_by_join_statement) .order(order_option) .order(sort_criteria_array) end diff --git a/app/models/query/results/group_by.rb b/app/models/query/results/group_by.rb index b5609d28e876..b6f829b24625 100644 --- a/app/models/query/results/group_by.rb +++ b/app/models/query/results/group_by.rb @@ -46,6 +46,7 @@ def work_package_count_for(group) def group_counts_by_group work_packages_with_includes_for_count + .joins(query.group_by_join_statement) .group(group_by_for_count) .visible .references(:statuses, :projects) diff --git a/app/models/query/results/sums.rb b/app/models/query/results/sums.rb index 23509ef6656f..27e735d4f204 100644 --- a/app/models/query/results/sums.rb +++ b/app/models/query/results/sums.rb @@ -86,7 +86,7 @@ def sums_work_package_scope(grouped) .select(sums_work_package_scope_selects(grouped)) if grouped - scope.group(query.group_by_statement) + scope.joins(query.group_by_join_statement).group(query.group_by_statement) else scope end From 70b1acbadcbdf63d83774065513a6e63a387fbb2 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Wed, 9 Oct 2024 19:58:07 +0200 Subject: [PATCH 13/28] fix join statement for group by to return ids instead of positions --- app/models/custom_field/order_statements.rb | 34 +++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 25401cb60cbc..b1f22c945343 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -75,6 +75,8 @@ def group_by_statement # Returns the join statement that is required to group objects by their value # of the custom field. def group_by_join_statement + return join_for_group_by_list_sql if field_format == "list" + order_join_statement end @@ -159,6 +161,38 @@ def join_for_order_by_list_sql end end + def join_for_group_by_list_sql + if multi_value? + <<-SQL.squish + LEFT OUTER JOIN ( + SELECT cv.customized_id, array_to_string(array_agg(cv.value ORDER BY co.position), '.') "value" + FROM #{CustomValue.quoted_table_name} cv + INNER JOIN #{CustomOption.quoted_table_name} co + ON co.id = cv.value::bigint + WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} + AND cv.custom_field_id = #{id} + AND cv.value IS NOT NULL + AND cv.value != '' + GROUP BY cv.customized_id + ) cf_order_#{id} + ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id + SQL + else + <<-SQL.squish + LEFT OUTER JOIN ( + SELECT DISTINCT ON (cv.customized_id) cv.customized_id, cv.value "value" + FROM #{CustomValue.quoted_table_name} cv + WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} + AND cv.custom_field_id = #{id} + AND cv.value IS NOT NULL + AND cv.value != '' + ORDER BY cv.customized_id, cv.id + ) cf_order_#{id} + ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id + SQL + end + end + def join_for_order_by_user_sql columns_array = "ARRAY[users.lastname, users.firstname, users.mail]" From 119de6ee8a52ee68396e01c2ce8f558d5d8f6a51 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Wed, 9 Oct 2024 19:58:52 +0200 Subject: [PATCH 14/28] handle absent values in group by Previously it relied on receiving empty string --- app/models/query/results/group_by.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/query/results/group_by.rb b/app/models/query/results/group_by.rb index b6f829b24625..59e8acbb0703 100644 --- a/app/models/query/results/group_by.rb +++ b/app/models/query/results/group_by.rb @@ -99,7 +99,7 @@ def transform_list_custom_field_keys(custom_field, groups) groups.transform_keys do |key| if custom_field.multi_value? - key.split(".").map do |subkey| + (key ? key.split(".") : []).map do |subkey| options[subkey].first end else @@ -109,7 +109,7 @@ def transform_list_custom_field_keys(custom_field, groups) end def custom_options_for_keys(custom_field, groups) - keys = groups.keys.map { |k| k.split(".") } + keys = groups.keys.map { |k| k ? k.split(".") : [] } # Because of multi select cfs we might end up having overlapping groups # (e.g group "1" and group "1.3" and group "3" which represent concatenated ids). # This can result in us having ids in the keys array multiple times (e.g. ["1", "1", "3", "3"]). From 8b3ceff83a159ff74660628103f28ef9e86a8f05 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 10 Oct 2024 14:38:08 +0200 Subject: [PATCH 15/28] pass group_by_join_statement to WorkPackageSelect.scoped_column_sum --- .../work_packages/selects/custom_field_select.rb | 2 +- .../work_packages/selects/work_package_select.rb | 10 ++++++---- modules/costs/lib/costs/query_currency_select.rb | 6 ++++-- .../costs/spec/lib/costs/query_currency_select_spec.rb | 8 ++++++++ 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/app/models/queries/work_packages/selects/custom_field_select.rb b/app/models/queries/work_packages/selects/custom_field_select.rb index 584dbfb44be9..28bc7339397b 100644 --- a/app/models/queries/work_packages/selects/custom_field_select.rb +++ b/app/models/queries/work_packages/selects/custom_field_select.rb @@ -89,7 +89,7 @@ def summable_statement ->(query, grouped) { Queries::WorkPackages::Selects::WorkPackageSelect - .scoped_column_sum(summable_scope(query), select, grouped && query.group_by_statement) + .scoped_column_sum(summable_scope(query), select, grouped:, query:) } else false diff --git a/app/models/queries/work_packages/selects/work_package_select.rb b/app/models/queries/work_packages/selects/work_package_select.rb index 947215515127..6caca492abbd 100644 --- a/app/models/queries/work_packages/selects/work_package_select.rb +++ b/app/models/queries/work_packages/selects/work_package_select.rb @@ -49,12 +49,14 @@ def self.select_group_by(group_by_statement) "#{group_by} id" end - def self.scoped_column_sum(scope, select, group_by) - scope = scope - .except(:order, :select) + def self.scoped_column_sum(scope, select, grouped:, query:) + scope = scope.except(:order, :select) + + if grouped + group_by = query.group_by_statement - if group_by scope + .joins(query.group_by_join_statement) .group(group_by) .select(select_group_by(group_by), select) else diff --git a/modules/costs/lib/costs/query_currency_select.rb b/modules/costs/lib/costs/query_currency_select.rb index f4d21aa6b19c..795da6bc500b 100644 --- a/modules/costs/lib/costs/query_currency_select.rb +++ b/modules/costs/lib/costs/query_currency_select.rb @@ -57,7 +57,8 @@ def real_value(work_package) Queries::WorkPackages::Selects::WorkPackageSelect .scoped_column_sum(scope, "COALESCE(ROUND(SUM(cost_entries_sum), 2)::FLOAT, 0.0) material_costs", - grouped && query.group_by_statement) + grouped:, + query:) } }, labor_costs: { @@ -70,7 +71,8 @@ def real_value(work_package) Queries::WorkPackages::Selects::WorkPackageSelect .scoped_column_sum(scope, "COALESCE(ROUND(SUM(time_entries_sum), 2)::FLOAT, 0.0) labor_costs", - grouped && query.group_by_statement) + grouped:, + query:) } }, overall_costs: { diff --git a/modules/costs/spec/lib/costs/query_currency_select_spec.rb b/modules/costs/spec/lib/costs/query_currency_select_spec.rb index 93da8b5e9886..d107207b9862 100644 --- a/modules/costs/spec/lib/costs/query_currency_select_spec.rb +++ b/modules/costs/spec/lib/costs/query_currency_select_spec.rb @@ -94,6 +94,10 @@ .to receive(:work_packages) .and_return(WorkPackage.all) + allow(query) + .to receive(:group_by_join_statement) + .and_return(nil) + allow(query) .to receive(:group_by_statement) .and_return("author_id") @@ -133,6 +137,10 @@ .to receive(:work_packages) .and_return(WorkPackage.all) + allow(query) + .to receive(:group_by_join_statement) + .and_return(nil) + allow(query) .to receive(:group_by_statement) .and_return("author_id") From 924e9c89e28437ee4015ddba0092615b0d817a03 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 10 Oct 2024 14:38:46 +0200 Subject: [PATCH 16/28] disambigute value in summable_select_statement --- .../queries/work_packages/selects/custom_field_select.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/queries/work_packages/selects/custom_field_select.rb b/app/models/queries/work_packages/selects/custom_field_select.rb index 28bc7339397b..8f4520d4f2a3 100644 --- a/app/models/queries/work_packages/selects/custom_field_select.rb +++ b/app/models/queries/work_packages/selects/custom_field_select.rb @@ -77,9 +77,9 @@ def summable_scope(query) def summable_select_statement if custom_field.field_format == "int" - "COALESCE(SUM(value::BIGINT)::BIGINT, 0) #{name}" + "COALESCE(SUM(#{CustomValue.quoted_table_name}.value::BIGINT)::BIGINT, 0) #{name}" else - "COALESCE(ROUND(SUM(value::NUMERIC), 2)::FLOAT, 0.0) #{name}" + "COALESCE(ROUND(SUM(#{CustomValue.quoted_table_name}.value::NUMERIC), 2)::FLOAT, 0.0) #{name}" end end From ac2c910638421f30321399a9309bab7afdd7b505 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 10 Oct 2024 16:18:06 +0200 Subject: [PATCH 17/28] use receive_messages as rubocop suggests --- .../lib/costs/query_currency_select_spec.rb | 32 ++++++------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/modules/costs/spec/lib/costs/query_currency_select_spec.rb b/modules/costs/spec/lib/costs/query_currency_select_spec.rb index d107207b9862..26ecf739d7de 100644 --- a/modules/costs/spec/lib/costs/query_currency_select_spec.rb +++ b/modules/costs/spec/lib/costs/query_currency_select_spec.rb @@ -86,21 +86,15 @@ query = double("query") result = double("result") - allow(query) - .to receive(:results) - .and_return result - allow(result) .to receive(:work_packages) .and_return(WorkPackage.all) - allow(query) - .to receive(:group_by_join_statement) - .and_return(nil) - - allow(query) - .to receive(:group_by_statement) - .and_return("author_id") + allow(query).to receive_messages( + results: result, + group_by_join_statement: nil, + group_by_statement: "author_id" + ) expect(ActiveRecord::Base.connection.select_all(instance.summable.(query, true).to_sql).columns) .to match_array %w(id material_costs) @@ -129,21 +123,15 @@ query = double("query") result = double("result") - allow(query) - .to receive(:results) - .and_return result - allow(result) .to receive(:work_packages) .and_return(WorkPackage.all) - allow(query) - .to receive(:group_by_join_statement) - .and_return(nil) - - allow(query) - .to receive(:group_by_statement) - .and_return("author_id") + allow(query).to receive_messages( + results: result, + group_by_join_statement: nil, + group_by_statement: "author_id" + ) expect(ActiveRecord::Base.connection.select_all(instance.summable.(query, true).to_sql).columns) .to match_array %w(id labor_costs) From ac6202b6bdba409247754d23950cb6635c661815 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 10 Oct 2024 17:50:16 +0200 Subject: [PATCH 18/28] deduplicate queries using template --- app/models/custom_field/order_statements.rb | 182 ++++---------------- 1 file changed, 32 insertions(+), 150 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index b1f22c945343..4f93dc338936 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -82,184 +82,66 @@ def group_by_join_statement private - def join_for_order_by_string_sql + def join_for_order_sql(value:, join: nil, multi_value: false) <<-SQL.squish LEFT OUTER JOIN ( - SELECT DISTINCT ON (cv.customized_id) cv.customized_id, cv.value "value" + SELECT + #{multi_value ? '' : 'DISTINCT ON (cv.customized_id)'} + cv.customized_id, + #{value} "value" FROM #{CustomValue.quoted_table_name} cv + #{join} WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} AND cv.custom_field_id = #{id} AND cv.value IS NOT NULL AND cv.value != '' - ORDER BY cv.customized_id, cv.id + #{multi_value ? 'GROUP BY cv.customized_id' : 'ORDER BY cv.customized_id, cv.id'} ) cf_order_#{id} ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id SQL end - def join_for_order_by_int_sql - <<-SQL.squish - LEFT OUTER JOIN ( - SELECT DISTINCT ON (cv.customized_id) cv.customized_id, cv.value::decimal(60) "value" - FROM #{CustomValue.quoted_table_name} cv - WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} - AND cv.custom_field_id = #{id} - AND cv.value IS NOT NULL - AND cv.value != '' - ORDER BY cv.customized_id, cv.id - ) cf_order_#{id} - ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id - SQL - end + def join_for_order_by_string_sql = join_for_order_sql(value: "cv.value") - def join_for_order_by_float_sql - <<-SQL.squish - LEFT OUTER JOIN ( - SELECT DISTINCT ON (cv.customized_id) cv.customized_id, cv.value::double precision "value" - FROM #{CustomValue.quoted_table_name} cv - WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} - AND cv.custom_field_id = #{id} - AND cv.value IS NOT NULL - AND cv.value != '' - ORDER BY cv.customized_id, cv.id - ) cf_order_#{id} - ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id - SQL - end + def join_for_order_by_int_sql = join_for_order_sql(value: "cv.value::decimal(60)") + + def join_for_order_by_float_sql = join_for_order_sql(value: "cv.value::double precision") def join_for_order_by_list_sql - if multi_value? - <<-SQL.squish - LEFT OUTER JOIN ( - SELECT cv.customized_id, array_agg(co.position ORDER BY co.position) "value" - FROM #{CustomValue.quoted_table_name} cv - INNER JOIN #{CustomOption.quoted_table_name} co - ON co.id = cv.value::bigint - WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} - AND cv.custom_field_id = #{id} - AND cv.value IS NOT NULL - AND cv.value != '' - GROUP BY cv.customized_id - ) cf_order_#{id} - ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id - SQL - else - <<-SQL.squish - LEFT OUTER JOIN ( - SELECT DISTINCT ON (cv.customized_id) cv.customized_id, co.position "value" - FROM #{CustomValue.quoted_table_name} cv - INNER JOIN #{CustomOption.quoted_table_name} co - ON co.id = cv.value::bigint - WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} - AND cv.custom_field_id = #{id} - AND cv.value IS NOT NULL - AND cv.value != '' - ORDER BY cv.customized_id, cv.id - ) cf_order_#{id} - ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id - SQL - end + join_for_order_sql( + value: multi_value? ? "ARRAY_AGG(co.position ORDER BY co.position)" : "co.position", + join: "INNER JOIN #{CustomOption.quoted_table_name} co ON co.id = cv.value::bigint", + multi_value: + ) end def join_for_group_by_list_sql if multi_value? - <<-SQL.squish - LEFT OUTER JOIN ( - SELECT cv.customized_id, array_to_string(array_agg(cv.value ORDER BY co.position), '.') "value" - FROM #{CustomValue.quoted_table_name} cv - INNER JOIN #{CustomOption.quoted_table_name} co - ON co.id = cv.value::bigint - WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} - AND cv.custom_field_id = #{id} - AND cv.value IS NOT NULL - AND cv.value != '' - GROUP BY cv.customized_id - ) cf_order_#{id} - ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id - SQL + join_for_order_sql( + value: "ARRAY_TO_STRING(ARRAY_AGG(cv.value ORDER BY co.position), '.')", + join: "INNER JOIN #{CustomOption.quoted_table_name} co ON co.id = cv.value::bigint", + multi_value: + ) else - <<-SQL.squish - LEFT OUTER JOIN ( - SELECT DISTINCT ON (cv.customized_id) cv.customized_id, cv.value "value" - FROM #{CustomValue.quoted_table_name} cv - WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} - AND cv.custom_field_id = #{id} - AND cv.value IS NOT NULL - AND cv.value != '' - ORDER BY cv.customized_id, cv.id - ) cf_order_#{id} - ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id - SQL + join_for_order_by_string_sql end end def join_for_order_by_user_sql columns_array = "ARRAY[users.lastname, users.firstname, users.mail]" - if multi_value? - <<-SQL.squish - LEFT OUTER JOIN ( - SELECT cv.customized_id, ARRAY_AGG(#{columns_array} ORDER BY #{columns_array}) "value" - FROM #{CustomValue.quoted_table_name} cv - INNER JOIN #{User.quoted_table_name} users - ON users.id = cv.value::bigint - WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} - AND cv.custom_field_id = #{id} - AND cv.value IS NOT NULL - AND cv.value != '' - GROUP BY cv.customized_id - ) cf_order_#{id} - ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id - SQL - else - <<-SQL.squish - LEFT OUTER JOIN ( - SELECT DISTINCT ON (cv.customized_id) cv.customized_id, #{columns_array} "value" - FROM #{CustomValue.quoted_table_name} cv - INNER JOIN #{User.quoted_table_name} users - ON users.id = cv.value::bigint - WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} - AND cv.custom_field_id = #{id} - AND cv.value IS NOT NULL - AND cv.value != '' - ORDER BY cv.customized_id, cv.id - ) cf_order_#{id} - ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id - SQL - end + join_for_order_sql( + value: multi_value? ? "ARRAY_AGG(#{columns_array} ORDER BY #{columns_array})" : columns_array, + join: "INNER JOIN #{User.quoted_table_name} users ON users.id = cv.value::bigint", + multi_value: + ) end def join_for_order_by_version_sql - if multi_value? - <<-SQL.squish - LEFT OUTER JOIN ( - SELECT cv.customized_id, array_agg(versions.name ORDER BY versions.name) "value" - FROM #{CustomValue.quoted_table_name} cv - INNER JOIN #{Version.quoted_table_name} versions - ON versions.id = cv.value::bigint - WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} - AND cv.custom_field_id = #{id} - AND cv.value IS NOT NULL - AND cv.value != '' - GROUP BY cv.customized_id - ) cf_order_#{id} - ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id - SQL - else - <<-SQL.squish - LEFT OUTER JOIN ( - SELECT DISTINCT ON (cv.customized_id) cv.customized_id, versions.name "value" - FROM #{CustomValue.quoted_table_name} cv - INNER JOIN #{Version.quoted_table_name} versions - ON versions.id = cv.value::bigint - WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} - AND cv.custom_field_id = #{id} - AND cv.value IS NOT NULL - AND cv.value != '' - ORDER BY cv.customized_id, cv.id - ) cf_order_#{id} - ON cf_order_#{id}.customized_id = #{self.class.customized_class.quoted_table_name}.id - SQL - end + join_for_order_sql( + value: multi_value? ? "array_agg(versions.name ORDER BY versions.name)" : "versions.name", + join: "INNER JOIN #{Version.quoted_table_name} versions ON versions.id = cv.value::bigint", + multi_value: + ) end end From 76e4096a81f788a31505406ba976b9fa532a57e2 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 10 Oct 2024 20:05:38 +0200 Subject: [PATCH 19/28] ensure group_by_join_statement is nil when group_by_statement is nil --- app/models/custom_field/order_statements.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 4f93dc338936..20f691f2fcbc 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -67,7 +67,7 @@ def order_null_handling(asc) # Returns the expression to use in GROUP BY (and ORDER BY) clause to group # objects by their value of the custom field. def group_by_statement - return unless field_format.in?(%w[list date bool int float string link]) + return unless can_be_used_for_grouping? order_statement end @@ -75,6 +75,8 @@ def group_by_statement # Returns the join statement that is required to group objects by their value # of the custom field. def group_by_join_statement + return unless can_be_used_for_grouping? + return join_for_group_by_list_sql if field_format == "list" order_join_statement @@ -82,6 +84,8 @@ def group_by_join_statement private + def can_be_used_for_grouping? = field_format.in?(%w[list date bool int float string link]) + def join_for_order_sql(value:, join: nil, multi_value: false) <<-SQL.squish LEFT OUTER JOIN ( From 526d287ad4ff61845cecf8c46ea45d8056f592d9 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 11 Oct 2024 13:26:33 +0200 Subject: [PATCH 20/28] order empty values before present values for all custom field formats --- app/models/custom_field/order_statements.rb | 2 -- .../project_query_custom_field_order_spec.rb | 26 +++++++++---------- .../results_cf_sorting_integration_spec.rb | 26 +++++++++---------- 3 files changed, 26 insertions(+), 28 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 20f691f2fcbc..7b99bb02980f 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -58,8 +58,6 @@ def order_join_statement # Returns the ORDER BY option defining order of objects without value for the # custom field. def order_null_handling(asc) - return unless %w[int float string date bool link].include?(field_format) - null_direction = asc ? "FIRST" : "LAST" Arel.sql("NULLS #{null_direction}") end diff --git a/spec/models/queries/projects/project_query_custom_field_order_spec.rb b/spec/models/queries/projects/project_query_custom_field_order_spec.rb index c430470ea6fe..87daf7783578 100644 --- a/spec/models/queries/projects/project_query_custom_field_order_spec.rb +++ b/spec/models/queries/projects/project_query_custom_field_order_spec.rb @@ -177,11 +177,11 @@ def project_without_cf_value let(:projects) do [ + project_without_cf_value, # sorting is done by position, and not by value project_with_cf_value(id_by_value.fetch("100")), project_with_cf_value(id_by_value.fetch("3")), project_with_cf_value(id_by_value.fetch("20")), - project_without_cf_value # TODO: should be at index 0 ] end end @@ -193,6 +193,7 @@ def project_without_cf_value let(:projects) do [ + project_without_cf_value, project_with_cf_value(*id_by_value.fetch_values("100")), # 100 project_with_cf_value(*id_by_value.fetch_values("3", "100")), # 100, 3 project_with_cf_value(*id_by_value.fetch_values("3", "20", "100")), # 100, 3, 20 @@ -205,14 +206,13 @@ def project_without_cf_value project_with_cf_value(*id_by_value.fetch_values("3")), # 3 project_with_cf_value(*id_by_value.fetch_values("3", "20")), # 3, 20 project_with_cf_value(*id_by_value.fetch_values("20")), # 20 - project_without_cf_value # TODO: decide on order of absent values ] end let(:projects_desc) do indexes = projects.each_index.to_a - # order of values for a work package is ignored, so ordered by falling back on id asc - indexes[2...8] = indexes[2...8].reverse + # order of work packages with same values in different order falls back on next column (id asc) + indexes[3...9] = indexes[3...9].reverse projects.values_at(*indexes.reverse) end end @@ -248,11 +248,11 @@ def project_without_cf_value let(:custom_field_values) do [ + nil, id_by_login.fetch("ax"), id_by_login.fetch("ba"), id_by_login.fetch("bb1"), id_by_login.fetch("bb2"), - nil # TODO: should be at index 0 ] end @@ -272,13 +272,13 @@ def project_without_cf_value let(:custom_field_values) do [ + [], id_by_login.fetch_values("ax"), # ax id_by_login.fetch_values("bb1", "ax"), # ax, bb1 id_by_login.fetch_values("ax", "bb1"), # ax, bb1 id_by_login.fetch_values("ba"), # ba id_by_login.fetch_values("bb1", "ba"), # ba, bb1 id_by_login.fetch_values("ba", "bb2"), # ba, bb2 - [] # TODO: should be at index 0 ] end @@ -290,8 +290,8 @@ def project_without_cf_value let(:projects_desc) do indexes = projects.each_index.to_a - # order of values for a work package is ignored, so ordered by falling back on id asc - indexes[1...3] = indexes[1...3].reverse + # order of work packages with same values in different order falls back on next column (id asc) + indexes[2...4] = indexes[2...4].reverse projects.values_at(*indexes.reverse) end end @@ -316,11 +316,11 @@ def project_without_cf_value let(:projects) do [ + project, project_with_cf_value(id_by_name.fetch("9")), project_with_cf_value(id_by_name.fetch("10.2")), project_with_cf_value(id_by_name.fetch("10.10.2")), project_with_cf_value(id_by_name.fetch("10.10.10")), - project # TODO: should be at index 0 ] end end @@ -332,20 +332,20 @@ def project_without_cf_value let(:projects) do [ + project, project_with_cf_value(*id_by_name.fetch_values("10.10.2", "9")), # 9, 10.10.2 project_with_cf_value(*id_by_name.fetch_values("10.10.10", "9")), # 9, 10.10.10 project_with_cf_value(*id_by_name.fetch_values("9", "10.10.10")), # 9, 10.10.10 project_with_cf_value(*id_by_name.fetch_values("10.2", "10.10.2")), # 10.2, 10.10.2 project_with_cf_value(*id_by_name.fetch_values("10.10.2")), # 10.10.2 - project_with_cf_value(*id_by_name.fetch_values("10.10.10")), # 10.10.10 - project # TODO: should be at index 0 + project_with_cf_value(*id_by_name.fetch_values("10.10.10")) # 10.10.10 ] end let(:projects_desc) do indexes = projects.each_index.to_a - # order of values for a work package is ignored, so ordered by falling back on id asc - indexes[1...3] = indexes[1...3].reverse + # order of work packages with same values in different order falls back on next column (id asc) + indexes[2...4] = indexes[2...4].reverse projects.values_at(*indexes.reverse) end end diff --git a/spec/models/query/results_cf_sorting_integration_spec.rb b/spec/models/query/results_cf_sorting_integration_spec.rb index 74e2f9865b39..461e9ba3c738 100644 --- a/spec/models/query/results_cf_sorting_integration_spec.rb +++ b/spec/models/query/results_cf_sorting_integration_spec.rb @@ -188,11 +188,11 @@ def wp_without_cf_value let(:work_packages) do [ + wp_without_cf_value, # sorting is done by position, and not by value wp_with_cf_value(id_by_value.fetch("100")), wp_with_cf_value(id_by_value.fetch("3")), wp_with_cf_value(id_by_value.fetch("20")), - wp_without_cf_value # TODO: should be at index 0 ] end end @@ -204,6 +204,7 @@ def wp_without_cf_value let(:work_packages) do [ + wp_without_cf_value, wp_with_cf_value(id_by_value.fetch_values("100")), # 100 wp_with_cf_value(id_by_value.fetch_values("3", "100")), # 100, 3 wp_with_cf_value(id_by_value.fetch_values("3", "20", "100")), # 100, 3, 20 @@ -216,14 +217,13 @@ def wp_without_cf_value wp_with_cf_value(id_by_value.fetch_values("3")), # 3 wp_with_cf_value(id_by_value.fetch_values("3", "20")), # 3, 20 wp_with_cf_value(id_by_value.fetch_values("20")), # 20 - wp_without_cf_value # TODO: decide on order of absent values ] end let(:work_packages_desc) do indexes = work_packages.each_index.to_a - # order of values for a project is ignored, so ordered by falling back on id asc - indexes[2...8] = indexes[2...8].reverse + # order of projects with same values in different order falls back on next column (id asc) + indexes[3...9] = indexes[3...9].reverse work_packages.values_at(*indexes.reverse) end end @@ -255,11 +255,11 @@ def wp_without_cf_value let(:work_packages) do [ + wp_without_cf_value, wp_with_cf_value(id_by_login.fetch("ax")), wp_with_cf_value(id_by_login.fetch("ba")), wp_with_cf_value(id_by_login.fetch("bb1")), wp_with_cf_value(id_by_login.fetch("bb2")), - wp_without_cf_value # TODO: should be at index 0 ] end end @@ -271,20 +271,20 @@ def wp_without_cf_value let(:work_packages) do [ + wp_without_cf_value, wp_with_cf_value(id_by_login.fetch_values("ax")), # ax wp_with_cf_value(id_by_login.fetch_values("bb1", "ax")), # ax, bb1 wp_with_cf_value(id_by_login.fetch_values("ax", "bb1")), # ax, bb1 wp_with_cf_value(id_by_login.fetch_values("ba")), # ba wp_with_cf_value(id_by_login.fetch_values("bb1", "ba")), # ba, bb1 wp_with_cf_value(id_by_login.fetch_values("ba", "bb2")), # ba, bb2 - wp_without_cf_value # TODO: should be at index 0 ] end let(:work_packages_desc) do indexes = work_packages.each_index.to_a - # order of values for a project is ignored, so ordered by falling back on id asc - indexes[1...3] = indexes[1...3].reverse + # order of projects with same values in different order falls back on next column (id asc) + indexes[2...4] = indexes[2...4].reverse work_packages.values_at(*indexes.reverse) end end @@ -308,11 +308,11 @@ def wp_without_cf_value let(:work_packages) do [ + wp_without_cf_value, wp_with_cf_value(id_by_name.fetch("9")), wp_with_cf_value(id_by_name.fetch("10.2")), wp_with_cf_value(id_by_name.fetch("10.10.2")), - wp_with_cf_value(id_by_name.fetch("10.10.10")), - wp_without_cf_value # TODO: should be at index 0 + wp_with_cf_value(id_by_name.fetch("10.10.10")) ] end end @@ -324,20 +324,20 @@ def wp_without_cf_value let(:work_packages) do [ + wp_without_cf_value, wp_with_cf_value(id_by_name.fetch_values("10.10.2", "9")), # 9, 10.10.2 wp_with_cf_value(id_by_name.fetch_values("10.10.10", "9")), # 9, 10.10.10 wp_with_cf_value(id_by_name.fetch_values("9", "10.10.10")), # 9, 10.10.10 wp_with_cf_value(id_by_name.fetch_values("10.2", "10.10.2")), # 10.2, 10.10.2 wp_with_cf_value(id_by_name.fetch_values("10.10.2")), # 10.10.2 wp_with_cf_value(id_by_name.fetch_values("10.10.10")), # 10.10.10 - wp_without_cf_value # TODO: should be at index 0 ] end let(:work_packages_desc) do indexes = work_packages.each_index.to_a - # order of values for a project is ignored, so ordered by falling back on id asc - indexes[1...3] = indexes[1...3].reverse + # order of projects with same values in different order falls back on next column (id asc) + indexes[2...4] = indexes[2...4].reverse work_packages.values_at(*indexes.reverse) end end From fbb246fe8d99213043260437c767ecc4ea69a91f Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Mon, 14 Oct 2024 18:32:28 +0200 Subject: [PATCH 21/28] fix order of list custom field groups to be same as when ordering --- app/models/custom_field/order_statements.rb | 30 ++++++++----------- .../selects/custom_field_select.rb | 1 + .../selects/work_package_select.rb | 1 + app/models/query.rb | 4 +++ app/models/query/results/group_by.rb | 2 +- 5 files changed, 20 insertions(+), 18 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 7b99bb02980f..a6a5062497a8 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -70,13 +70,19 @@ def group_by_statement order_statement end + # Returns the expression to use in SELECT clause if it differs from one used + # to group by + def group_by_select_statement + return unless field_format == "list" + + "ANY_VALUE(cf_order_#{id}.ids)" + end + # Returns the join statement that is required to group objects by their value # of the custom field. def group_by_join_statement return unless can_be_used_for_grouping? - return join_for_group_by_list_sql if field_format == "list" - order_join_statement end @@ -84,13 +90,14 @@ def group_by_join_statement def can_be_used_for_grouping? = field_format.in?(%w[list date bool int float string link]) - def join_for_order_sql(value:, join: nil, multi_value: false) + def join_for_order_sql(value:, add_select: nil, join: nil, multi_value: false) <<-SQL.squish LEFT OUTER JOIN ( SELECT #{multi_value ? '' : 'DISTINCT ON (cv.customized_id)'} - cv.customized_id, - #{value} "value" + cv.customized_id + , #{value} "value" + #{", #{add_select}" if add_select} FROM #{CustomValue.quoted_table_name} cv #{join} WHERE cv.customized_type = #{CustomValue.connection.quote(self.class.customized_class.name)} @@ -112,23 +119,12 @@ def join_for_order_by_float_sql = join_for_order_sql(value: "cv.value::double pr def join_for_order_by_list_sql join_for_order_sql( value: multi_value? ? "ARRAY_AGG(co.position ORDER BY co.position)" : "co.position", + add_select: "#{multi_value? ? "ARRAY_TO_STRING(ARRAY_AGG(cv.value ORDER BY co.position), '.')" : 'cv.value'} ids", join: "INNER JOIN #{CustomOption.quoted_table_name} co ON co.id = cv.value::bigint", multi_value: ) end - def join_for_group_by_list_sql - if multi_value? - join_for_order_sql( - value: "ARRAY_TO_STRING(ARRAY_AGG(cv.value ORDER BY co.position), '.')", - join: "INNER JOIN #{CustomOption.quoted_table_name} co ON co.id = cv.value::bigint", - multi_value: - ) - else - join_for_order_by_string_sql - end - end - def join_for_order_by_user_sql columns_array = "ARRAY[users.lastname, users.firstname, users.mail]" diff --git a/app/models/queries/work_packages/selects/custom_field_select.rb b/app/models/queries/work_packages/selects/custom_field_select.rb index 8f4520d4f2a3..6120e1111035 100644 --- a/app/models/queries/work_packages/selects/custom_field_select.rb +++ b/app/models/queries/work_packages/selects/custom_field_select.rb @@ -37,6 +37,7 @@ def initialize(custom_field) @sortable_join = custom_field.order_join_statement @groupable = custom_field.group_by_statement @groupable_join = custom_field.group_by_join_statement + @groupable_select = custom_field.group_by_select_statement @summable = summable_statement end diff --git a/app/models/queries/work_packages/selects/work_package_select.rb b/app/models/queries/work_packages/selects/work_package_select.rb index 6caca492abbd..f52df5347d9f 100644 --- a/app/models/queries/work_packages/selects/work_package_select.rb +++ b/app/models/queries/work_packages/selects/work_package_select.rb @@ -31,6 +31,7 @@ class Queries::WorkPackages::Selects::WorkPackageSelect :name, :sortable_join, :groupable_join, + :groupable_select, :summable, :default_order, :association, diff --git a/app/models/query.rb b/app/models/query.rb index cd071156a38c..47eabd5b19d6 100644 --- a/app/models/query.rb +++ b/app/models/query.rb @@ -362,6 +362,10 @@ def group_by_statement group_by_column&.groupable end + def group_by_select + group_by_column&.groupable_select || group_by_statement + end + def group_by_join_statement group_by_column&.groupable_join end diff --git a/app/models/query/results/group_by.rb b/app/models/query/results/group_by.rb index 59e8acbb0703..c3097fdb03e7 100644 --- a/app/models/query/results/group_by.rb +++ b/app/models/query/results/group_by.rb @@ -68,7 +68,7 @@ def group_by_for_count end def pluck_for_count - Array(query.group_by_statement).map { |statement| Arel.sql(statement) } + + Array(query.group_by_select).map { |statement| Arel.sql(statement) } + [Arel.sql('COUNT(DISTINCT "work_packages"."id")')] end From ae6702adefae133dc994e2e6d27d4cf359863062 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Mon, 14 Oct 2024 18:49:35 +0200 Subject: [PATCH 22/28] use MIN instead of ANY_VALUE, as latter is available only in PostgreSQL 16 --- app/models/custom_field/order_statements.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index a6a5062497a8..464f3ed6b15c 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -75,7 +75,9 @@ def group_by_statement def group_by_select_statement return unless field_format == "list" - "ANY_VALUE(cf_order_#{id}.ids)" + # MIN needed to not add this column to group by, ANY_VALUE can be used when + # minimum required PostgreSQL becomes 16 + "MIN(cf_order_#{id}.ids)" end # Returns the join statement that is required to group objects by their value From e5082681d5356bc983f0f4f56dc041e187090fcd Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Tue, 15 Oct 2024 15:58:08 +0200 Subject: [PATCH 23/28] fix columns used in summing --- .../queries/work_packages/selects/work_package_select.rb | 6 ++---- app/models/query/results/sums.rb | 2 +- modules/costs/spec/lib/costs/query_currency_select_spec.rb | 2 ++ 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/queries/work_packages/selects/work_package_select.rb b/app/models/queries/work_packages/selects/work_package_select.rb index f52df5347d9f..90ed98e5f530 100644 --- a/app/models/queries/work_packages/selects/work_package_select.rb +++ b/app/models/queries/work_packages/selects/work_package_select.rb @@ -54,12 +54,10 @@ def self.scoped_column_sum(scope, select, grouped:, query:) scope = scope.except(:order, :select) if grouped - group_by = query.group_by_statement - scope .joins(query.group_by_join_statement) - .group(group_by) - .select(select_group_by(group_by), select) + .group(query.group_by_statement) + .select(select_group_by(query.group_by_select), select) else scope .select(select) diff --git a/app/models/query/results/sums.rb b/app/models/query/results/sums.rb index 27e735d4f204..d349e199f2f9 100644 --- a/app/models/query/results/sums.rb +++ b/app/models/query/results/sums.rb @@ -109,7 +109,7 @@ def sums_callable_joins(grouped) def sums_work_package_scope_selects(grouped) group_statement = if grouped - [Queries::WorkPackages::Selects::WorkPackageSelect.select_group_by(query.group_by_statement)] + [Queries::WorkPackages::Selects::WorkPackageSelect.select_group_by(query.group_by_select)] else [] end diff --git a/modules/costs/spec/lib/costs/query_currency_select_spec.rb b/modules/costs/spec/lib/costs/query_currency_select_spec.rb index 26ecf739d7de..8139e7f1c884 100644 --- a/modules/costs/spec/lib/costs/query_currency_select_spec.rb +++ b/modules/costs/spec/lib/costs/query_currency_select_spec.rb @@ -93,6 +93,7 @@ allow(query).to receive_messages( results: result, group_by_join_statement: nil, + group_by_select: "author_id", group_by_statement: "author_id" ) @@ -130,6 +131,7 @@ allow(query).to receive_messages( results: result, group_by_join_statement: nil, + group_by_select: "author_id", group_by_statement: "author_id" ) From 7e0ed443a80413fa74b00ab4e8b759acbeca3ee2 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Tue, 15 Oct 2024 17:54:02 +0200 Subject: [PATCH 24/28] use group_id instead of id to name the column used to group from summing --- .../queries/work_packages/selects/work_package_select.rb | 2 +- app/models/query/results/sums.rb | 9 +++++---- .../costs/spec/lib/costs/query_currency_select_spec.rb | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/app/models/queries/work_packages/selects/work_package_select.rb b/app/models/queries/work_packages/selects/work_package_select.rb index 90ed98e5f530..9b9603518367 100644 --- a/app/models/queries/work_packages/selects/work_package_select.rb +++ b/app/models/queries/work_packages/selects/work_package_select.rb @@ -47,7 +47,7 @@ def self.select_group_by(group_by_statement) group_by = group_by_statement group_by = group_by.first if group_by.is_a?(Array) - "#{group_by} id" + "#{group_by} group_id" end def self.scoped_column_sum(scope, select, grouped:, query:) diff --git a/app/models/query/results/sums.rb b/app/models/query/results/sums.rb index d349e199f2f9..6da74e8cfba6 100644 --- a/app/models/query/results/sums.rb +++ b/app/models/query/results/sums.rb @@ -43,10 +43,10 @@ def all_group_sums return nil unless query.grouped? sums_by_id = sums_select(true).inject({}) do |result, group_sum| - result[group_sum["id"]] = {} + result[group_sum["group_id"]] = {} query.summed_up_columns.each do |column| - result[group_sum["id"]][column] = group_sum[column.name.to_s] + result[group_sum["group_id"]][column] = group_sum[column.name.to_s] end result @@ -59,7 +59,7 @@ def all_group_sums def sums_select(grouped = false) select = if grouped - ["work_packages.id"] + ["work_packages.group_id"] else [] end @@ -96,7 +96,8 @@ def sums_callable_joins(grouped) callable_summed_up_columns .map do |c| join_condition = if grouped - "#{c.name}.id = work_packages.id OR #{c.name}.id IS NULL AND work_packages.id IS NULL" + "#{c.name}.group_id = work_packages.group_id OR " \ + "#{c.name}.group_id IS NULL AND work_packages.group_id IS NULL" else "TRUE" end diff --git a/modules/costs/spec/lib/costs/query_currency_select_spec.rb b/modules/costs/spec/lib/costs/query_currency_select_spec.rb index 8139e7f1c884..90794f8d2985 100644 --- a/modules/costs/spec/lib/costs/query_currency_select_spec.rb +++ b/modules/costs/spec/lib/costs/query_currency_select_spec.rb @@ -98,7 +98,7 @@ ) expect(ActiveRecord::Base.connection.select_all(instance.summable.(query, true).to_sql).columns) - .to match_array %w(id material_costs) + .to match_array %w(group_id material_costs) end end end @@ -136,7 +136,7 @@ ) expect(ActiveRecord::Base.connection.select_all(instance.summable.(query, true).to_sql).columns) - .to match_array %w(id labor_costs) + .to match_array %w(group_id labor_costs) end end end From 9c99bc8cd9c3da0e4184f34198e72a9acdbaff53 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Tue, 15 Oct 2024 19:24:54 +0200 Subject: [PATCH 25/28] extract method to calm rubocop --- app/models/query/results/sums.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/app/models/query/results/sums.rb b/app/models/query/results/sums.rb index 6da74e8cfba6..3eaf4dec7cca 100644 --- a/app/models/query/results/sums.rb +++ b/app/models/query/results/sums.rb @@ -42,7 +42,13 @@ def all_total_sums def all_group_sums return nil unless query.grouped? - sums_by_id = sums_select(true).inject({}) do |result, group_sum| + transform_group_keys(sums_by_group_id) + end + + private + + def sums_by_group_id + sums_select(true).inject({}) do |result, group_sum| result[group_sum["group_id"]] = {} query.summed_up_columns.each do |column| @@ -51,12 +57,8 @@ def all_group_sums result end - - transform_group_keys(sums_by_id) end - private - def sums_select(grouped = false) select = if grouped ["work_packages.group_id"] From bb9b32e749f0af4bf0ac99bcc52f98d5ce9373d9 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 18 Oct 2024 19:23:09 +0200 Subject: [PATCH 26/28] add resulting join sqls from template in comment --- app/models/custom_field/order_statements.rb | 23 +++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 464f3ed6b15c..86984954feea 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -92,6 +92,29 @@ def group_by_join_statement def can_be_used_for_grouping? = field_format.in?(%w[list date bool int float string link]) + # Template for all the join statements. + # + # For single value custom fields the join ensures single value for every + # customized object using DISTINCT ON and selecting first value by id of + # custom value: + # + # LEFT OUTER JOIN ( + # SELECT DISTINCT ON (cv.customized_id), cv.customized_id, xxx "value" + # FROM custom_values cv + # WHERE … + # ORDER BY cv.customized_id, cv.id + # ) cf_order_NNN ON cf_order_NNN.customized_id = … + # + # For multi value custom fields the GROUP BY and value aggregate function + # ensure single value for every customized object: + # + # LEFT OUTER JOIN ( + # SELECT cv.customized_id, ARRAY_AGG(xxx ORDERY BY yyy) "value" + # FROM custom_values cv + # WHERE … + # GROUP BY cv.customized_id, cv.id + # ) cf_order_NNN ON cf_order_NNN.customized_id = … + # def join_for_order_sql(value:, add_select: nil, join: nil, multi_value: false) <<-SQL.squish LEFT OUTER JOIN ( From 98ca48cac4fbdf40a9ecf7cfa9c4f360c2060c94 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Mon, 11 Nov 2024 19:01:03 +0100 Subject: [PATCH 27/28] fix link strategy to return nil when value is not set --- app/models/custom_value/link_strategy.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/custom_value/link_strategy.rb b/app/models/custom_value/link_strategy.rb index 3e6951d6dec7..091c01de5d63 100644 --- a/app/models/custom_value/link_strategy.rb +++ b/app/models/custom_value/link_strategy.rb @@ -28,7 +28,7 @@ class CustomValue::LinkStrategy < CustomValue::FormatStrategy def typed_value - formatted_value + formatted_value if value.present? end def parse_value(val) From dc28e44b5e932542d9e97f5f4e0a899bb46dd2e2 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Wed, 13 Nov 2024 14:59:03 +0100 Subject: [PATCH 28/28] move creation of angular boostrap __ng2-bootstrap-has-run mark, so it is set after turbo:load too --- frontend/src/app/app.module.ts | 2 ++ frontend/src/main.ts | 6 +----- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/frontend/src/app/app.module.ts b/frontend/src/app/app.module.ts index a35c22a3565c..51d140173014 100644 --- a/frontend/src/app/app.module.ts +++ b/frontend/src/app/app.module.ts @@ -414,6 +414,8 @@ export class OpenProjectModule implements DoBootstrap { if (root) { appRef.bootstrap(ApplicationBaseComponent, root); } + + document.body.classList.add('__ng2-bootstrap-has-run'); } private registerCustomElements(injector:Injector) { diff --git a/frontend/src/main.ts b/frontend/src/main.ts index 51373fe7ccad..82f7151eae7f 100644 --- a/frontend/src/main.ts +++ b/frontend/src/main.ts @@ -44,10 +44,6 @@ void initializeLocale() initializeGlobalListeners(); // Due to the behaviour of the Edge browser we need to wait for 'DOM ready' - void platformBrowserDynamic() - .bootstrapModule(OpenProjectModule) - .then(() => { - jQuery('body').addClass('__ng2-bootstrap-has-run'); - }); + void platformBrowserDynamic().bootstrapModule(OpenProjectModule); }); });