From c695101b5ec224c738e72c2e919385f31c674b5c Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Tue, 17 Sep 2024 15:24:10 +0200 Subject: [PATCH 01/16] [#57577] Broken ordering by multi value custom fields https://community.openproject.org/work_packages/57577 From 2047b3d7c965d6759d4eb94a00cdc8bec1504e44 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Tue, 17 Sep 2024 15:53:51 +0200 Subject: [PATCH 02/16] use array to order by user custom field instead of 3 order statements Switch to using mail instead of id as agreed, as array can't contain different types and mail is an unique string. This should not be a security concern, as minimal information is exposed --- app/models/custom_field/order_statements.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index c4be1d1fc81c..2c6e3a45558e 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -46,11 +46,7 @@ def order_statements # CustomValue validations should ensure that it doesn't occur [select_custom_value_as_decimal] when "user" - [ - order_by_user_sql("lastname"), - order_by_user_sql("firstname"), - order_by_user_sql("id") - ] + [order_by_user_sql] when "version" [order_by_version_sql("name")] end @@ -134,9 +130,9 @@ def select_custom_value_as_decimal SQL end - def order_by_user_sql(column) + def order_by_user_sql <<-SQL - (SELECT #{column} user_cv_#{column} FROM #{User.table_name} cv_user + (SELECT ARRAY[cv_user.lastname, cv_user.firstname, cv_user.mail] FROM #{User.table_name} cv_user WHERE cv_user.id = #{select_custom_value_as_decimal} LIMIT 1) SQL From a190ef05c0e48a7a704c5e501560f9ff53a58a30 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Tue, 17 Sep 2024 15:55:48 +0200 Subject: [PATCH 03/16] use quoted_table_name instead of table_name --- app/models/custom_field/order_statements.rb | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 2c6e3a45558e..d073bff333d2 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -86,7 +86,7 @@ def coalesce_select_custom_value_as_string def select_custom_value_as_string <<-SQL - (SELECT cv_sort.value FROM #{CustomValue.table_name} cv_sort + (SELECT cv_sort.value FROM #{CustomValue.quoted_table_name} cv_sort WHERE #{cv_sort_only_custom_field_condition_sql} LIMIT 1) SQL @@ -94,8 +94,8 @@ def select_custom_value_as_string def select_custom_option_position <<-SQL - (SELECT co_sort.position FROM #{CustomOption.table_name} co_sort - LEFT JOIN #{CustomValue.table_name} cv_sort + (SELECT co_sort.position FROM #{CustomOption.quoted_table_name} co_sort + LEFT JOIN #{CustomValue.quoted_table_name} cv_sort ON co_sort.id = CAST(cv_sort.value AS decimal(60,3)) WHERE #{cv_sort_only_custom_field_condition_sql} LIMIT 1 @@ -105,7 +105,7 @@ def select_custom_option_position def select_custom_values_as_group <<-SQL - COALESCE((SELECT string_agg(cv_sort.value, '.') FROM #{CustomValue.table_name} cv_sort + 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 @@ -113,8 +113,8 @@ def select_custom_values_as_group def select_custom_values_joined_options_as_group <<-SQL - COALESCE((SELECT string_agg(co_sort.value, '.' ORDER BY co_sort.position ASC) FROM #{CustomOption.table_name} co_sort - LEFT JOIN #{CustomValue.table_name} cv_sort + COALESCE((SELECT string_agg(co_sort.value, '.' ORDER BY co_sort.position ASC) FROM #{CustomOption.quoted_table_name} co_sort + LEFT JOIN #{CustomValue.quoted_table_name} cv_sort ON cv_sort.value IS NOT NULL AND co_sort.id = cv_sort.value::numeric WHERE #{cv_sort_only_custom_field_condition_sql}), '') SQL @@ -122,7 +122,7 @@ def select_custom_values_joined_options_as_group def select_custom_value_as_decimal <<-SQL - (SELECT CAST(cv_sort.value AS decimal(60,3)) FROM #{CustomValue.table_name} cv_sort + (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 @@ -132,7 +132,7 @@ def select_custom_value_as_decimal def order_by_user_sql <<-SQL - (SELECT ARRAY[cv_user.lastname, cv_user.firstname, cv_user.mail] FROM #{User.table_name} cv_user + (SELECT ARRAY[cv_user.lastname, cv_user.firstname, cv_user.mail] FROM #{User.quoted_table_name} cv_user WHERE cv_user.id = #{select_custom_value_as_decimal} LIMIT 1) SQL @@ -140,7 +140,7 @@ def order_by_user_sql def order_by_version_sql(column) <<-SQL - (SELECT #{column} version_cv_#{column} FROM #{Version.table_name} cv_version + (SELECT #{column} version_cv_#{column} FROM #{Version.quoted_table_name} cv_version WHERE cv_version.id = #{select_custom_value_as_decimal} LIMIT 1) SQL @@ -149,7 +149,7 @@ def order_by_version_sql(column) def cv_sort_only_custom_field_condition_sql <<-SQL cv_sort.customized_type='#{self.class.customized_class.name}' - AND cv_sort.customized_id=#{self.class.customized_class.table_name}.id + AND cv_sort.customized_id=#{self.class.customized_class.quoted_table_name}.id AND cv_sort.custom_field_id=#{id} SQL end From d25bec9d5fd8b310cee512ad492b1d9dedd553e4 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 19 Sep 2024 14:51:09 +0200 Subject: [PATCH 04/16] inline version column as it is always and only name --- app/models/custom_field/order_statements.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index d073bff333d2..0802bdcccedc 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -48,7 +48,7 @@ def order_statements when "user" [order_by_user_sql] when "version" - [order_by_version_sql("name")] + [order_by_version_sql] end end @@ -138,9 +138,9 @@ def order_by_user_sql SQL end - def order_by_version_sql(column) + def order_by_version_sql <<-SQL - (SELECT #{column} version_cv_#{column} FROM #{Version.quoted_table_name} cv_version + (SELECT cv_version.name FROM #{Version.quoted_table_name} cv_version WHERE cv_version.id = #{select_custom_value_as_decimal} LIMIT 1) SQL From c8639a39d0ebb6a3bcd2f75a51e55f058d187541 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 19 Sep 2024 19:11:37 +0200 Subject: [PATCH 05/16] eq_array matcher to be able to see a diff --- spec/support/matchers/eq_array.rb | 52 +++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 spec/support/matchers/eq_array.rb diff --git a/spec/support/matchers/eq_array.rb b/spec/support/matchers/eq_array.rb new file mode 100644 index 000000000000..eb88336cc1ea --- /dev/null +++ b/spec/support/matchers/eq_array.rb @@ -0,0 +1,52 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +# Differ is private, so this can break at any moment +# Example: +# expect([1, 2, 3]).to eq_array [2, 3, 4] +# expect(actual).to eq_array(expected) { [_1.id, _1.value] } +RSpec::Matchers.define :eq_array do |expected| + match { |actual| expected == actual } + + failure_message do |actual| + actual_values = block_arg ? actual.map(&block_arg) : actual + expected_values = block_arg ? expected.map(&block_arg) : expected + + diff = RSpec::Expectations.differ.diff( + actual_values.map(&:inspect).join("\n"), + expected_values.map(&:inspect).join("\n") + ) + + <<~MESSAGE + expected: #{expected_values} + got: #{actual_values} + + #{diff} + MESSAGE + end +end From 5be7a1d9307fd10ecbe839ff5e06d9fb4faf49ad Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 19 Sep 2024 19:13:05 +0200 Subject: [PATCH 06/16] use eq_array matcher to see difference in order when ordering by custom field is unexpected --- .../project_query_custom_field_order_spec.rb | 13 +++++++++---- .../query/results_cf_sorting_integration_spec.rb | 13 +++++++++---- 2 files changed, 18 insertions(+), 8 deletions(-) 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 9933b52b024b..ce7b5eba85de 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 @@ -58,12 +58,18 @@ def project_without_cf_value before { projects } + project_attributes = ->(project) do + { + id: project.id, + values: project.custom_values.map(&:value).sort + } + end + context "in ascending order" do let(:direction) { :asc } it "returns the correctly sorted result" do - expect(query_results.map(&:id)) - .to eq projects.map(&:id) + expect(query_results).to eq_array(projects, &project_attributes) end end @@ -71,8 +77,7 @@ def project_without_cf_value let(:direction) { :desc } it "returns the correctly sorted result" do - expect(query_results.map(&:id)) - .to eq projects_desc.map(&:id) + expect(query_results).to eq_array(projects_desc, &project_attributes) end 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 1f992fb74a15..668cebcdcb70 100644 --- a/spec/models/query/results_cf_sorting_integration_spec.rb +++ b/spec/models/query/results_cf_sorting_integration_spec.rb @@ -69,12 +69,18 @@ def wp_without_cf_value before { work_packages } + work_package_attributes = ->(work_package) do + { + id: work_package.id, + values: work_package.custom_values.map(&:value).sort + } + end + context "in ascending order" do let(:sort_criteria) { [[custom_field.column_name, "asc"], %w[id asc]] } it "returns the correctly sorted result" do - expect(query_results.work_packages.map(&:id)) - .to eq work_packages.map(&:id) + expect(query_results.work_packages).to eq_array(work_packages, &work_package_attributes) end end @@ -82,8 +88,7 @@ def wp_without_cf_value let(:sort_criteria) { [[custom_field.column_name, "desc"], %w[id asc]] } it "returns the correctly sorted result" do - expect(query_results.work_packages.map(&:id)) - .to eq work_package_desc.map(&:id) + expect(query_results.work_packages).to eq_array(work_package_desc, &work_package_attributes) end end end From 749020dfa9e39a42dff091eef5502865888d0ff3 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 19 Sep 2024 19:53:12 +0200 Subject: [PATCH 07/16] fix ordering of multi value list custom fields --- app/models/custom_field/order_statements.rb | 45 ++++++++++--------- .../project_query_custom_field_order_spec.rb | 17 ++++--- .../results_cf_sorting_integration_spec.rb | 17 ++++--- 3 files changed, 39 insertions(+), 40 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 0802bdcccedc..013e57d67014 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -33,11 +33,7 @@ module CustomField::OrderStatements def order_statements case field_format when "list" - if multi_value? - [select_custom_values_joined_options_as_group] - else - [select_custom_option_position] - end + [select_custom_option_position] when "string", "date", "bool", "link" [coalesce_select_custom_value_as_string] when "int", "float" @@ -93,14 +89,28 @@ def select_custom_value_as_string end def select_custom_option_position - <<-SQL - (SELECT co_sort.position FROM #{CustomOption.quoted_table_name} co_sort - LEFT JOIN #{CustomValue.quoted_table_name} cv_sort - ON co_sort.id = CAST(cv_sort.value AS decimal(60,3)) - WHERE #{cv_sort_only_custom_field_condition_sql} - LIMIT 1 - ) - SQL + if multi_value? + <<-SQL.squish + ( + SELECT array_agg(co_sort.position ORDER BY co_sort.position) + FROM #{CustomOption.quoted_table_name} co_sort + LEFT JOIN #{CustomValue.quoted_table_name} cv_sort + ON cv_sort.value IS NOT NULL AND co_sort.id = cv_sort.value::bigint + WHERE #{cv_sort_only_custom_field_condition_sql} + ) + SQL + else + <<-SQL.squish + ( + SELECT co_sort.position + FROM #{CustomOption.quoted_table_name} co_sort + LEFT JOIN #{CustomValue.quoted_table_name} cv_sort + ON cv_sort.value IS NOT NULL AND co_sort.id = cv_sort.value::bigint + WHERE #{cv_sort_only_custom_field_condition_sql} + LIMIT 1 + ) + SQL + end end def select_custom_values_as_group @@ -111,15 +121,6 @@ def select_custom_values_as_group SQL end - def select_custom_values_joined_options_as_group - <<-SQL - COALESCE((SELECT string_agg(co_sort.value, '.' ORDER BY co_sort.position ASC) FROM #{CustomOption.quoted_table_name} co_sort - LEFT JOIN #{CustomValue.quoted_table_name} cv_sort - ON cv_sort.value IS NOT NULL AND co_sort.id = cv_sort.value::numeric - WHERE #{cv_sort_only_custom_field_condition_sql}), '') - SQL - end - def select_custom_value_as_decimal <<-SQL (SELECT CAST(cv_sort.value AS decimal(60,3)) FROM #{CustomValue.quoted_table_name} cv_sort 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 ce7b5eba85de..5ac0873957c8 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 @@ -193,15 +193,14 @@ def project_without_cf_value let(:projects) do [ - project_without_cf_value, - # TODO: sorting is done by values sorted by position and joined by `.`, why? - project_with_cf_value(*id_by_value.fetch_values("100")), # => 100 - project_with_cf_value(*id_by_value.fetch_values("20", "100")), # => 100.20 - project_with_cf_value(*id_by_value.fetch_values("3", "100")), # => 100.3 - project_with_cf_value(*id_by_value.fetch_values("100", "3", "20")), # => 100.3.20 - project_with_cf_value(*id_by_value.fetch_values("20")), # => 20 - 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("100")), + project_with_cf_value(*id_by_value.fetch_values("3", "100")), + project_with_cf_value(*id_by_value.fetch_values("100", "3", "20")), + project_with_cf_value(*id_by_value.fetch_values("20", "100")), + project_with_cf_value(*id_by_value.fetch_values("3")), + project_with_cf_value(*id_by_value.fetch_values("3", "20")), + project_with_cf_value(*id_by_value.fetch_values("20")), + project_without_cf_value # TODO: decide on order of absent values ] 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 668cebcdcb70..93bfb74f138b 100644 --- a/spec/models/query/results_cf_sorting_integration_spec.rb +++ b/spec/models/query/results_cf_sorting_integration_spec.rb @@ -204,15 +204,14 @@ def wp_without_cf_value let(:work_packages) do [ - wp_without_cf_value, - # TODO: sorting is done by values sorted by position and joined by `.`, why? - wp_with_cf_value(id_by_value.fetch_values("100")), # => 100 - wp_with_cf_value(id_by_value.fetch_values("20", "100")), # => 100.20 - wp_with_cf_value(id_by_value.fetch_values("3", "100")), # => 100.3 - wp_with_cf_value(id_by_value.fetch_values("100", "3", "20")), # => 100.3.20 - wp_with_cf_value(id_by_value.fetch_values("20")), # => 20 - 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("100")), + wp_with_cf_value(id_by_value.fetch_values("3", "100")), + wp_with_cf_value(id_by_value.fetch_values("100", "3", "20")), + wp_with_cf_value(id_by_value.fetch_values("20", "100")), + wp_with_cf_value(id_by_value.fetch_values("3")), + wp_with_cf_value(id_by_value.fetch_values("3", "20")), + wp_with_cf_value(id_by_value.fetch_values("20")), + wp_without_cf_value # TODO: decide on order of absent values ] end end From 8d916a2b1b734138646c90d6a8e4b0c5f83dbbb9 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 19 Sep 2024 20:05:37 +0200 Subject: [PATCH 08/16] fix variable name --- spec/models/query/results_cf_sorting_integration_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/models/query/results_cf_sorting_integration_spec.rb b/spec/models/query/results_cf_sorting_integration_spec.rb index 93bfb74f138b..f7410b6d69ce 100644 --- a/spec/models/query/results_cf_sorting_integration_spec.rb +++ b/spec/models/query/results_cf_sorting_integration_spec.rb @@ -65,7 +65,7 @@ def wp_without_cf_value end shared_examples "it sorts" do - let(:work_package_desc) { work_packages.reverse } + let(:work_packages_desc) { work_packages.reverse } before { work_packages } @@ -88,7 +88,7 @@ def wp_without_cf_value let(:sort_criteria) { [[custom_field.column_name, "desc"], %w[id asc]] } it "returns the correctly sorted result" do - expect(query_results.work_packages).to eq_array(work_package_desc, &work_package_attributes) + expect(query_results.work_packages).to eq_array(work_packages_desc, &work_package_attributes) end end end @@ -269,7 +269,7 @@ def wp_without_cf_value end # TODO: second user is ignored, so order due to falling back on id asc - let(:work_package_desc) { work_packages.values_at(4, 2, 3, 1, 0) } + let(:work_packages_desc) { work_packages.values_at(4, 2, 3, 1, 0) } end end end @@ -317,7 +317,7 @@ def wp_without_cf_value end # TODO: second version is ignored, so order due to falling back on id asc - let(:work_package_desc) { work_packages.values_at(4, 2, 3, 1, 0) } + let(:work_packages_desc) { work_packages.values_at(4, 2, 3, 1, 0) } end end end From e0b1cb61782138ed66c96126299cb44b5535021e Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 19 Sep 2024 20:06:30 +0200 Subject: [PATCH 09/16] test ordering of objects with same values in different order --- .../project_query_custom_field_order_spec.rb | 26 ++++++++++++++----- .../results_cf_sorting_integration_spec.rb | 26 ++++++++++++++----- 2 files changed, 38 insertions(+), 14 deletions(-) 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 5ac0873957c8..e47e4fa9f05f 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 @@ -193,16 +193,28 @@ def project_without_cf_value let(:projects) do [ - project_with_cf_value(*id_by_value.fetch_values("100")), - project_with_cf_value(*id_by_value.fetch_values("3", "100")), - project_with_cf_value(*id_by_value.fetch_values("100", "3", "20")), - project_with_cf_value(*id_by_value.fetch_values("20", "100")), - project_with_cf_value(*id_by_value.fetch_values("3")), - project_with_cf_value(*id_by_value.fetch_values("3", "20")), - project_with_cf_value(*id_by_value.fetch_values("20")), + 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 + project_with_cf_value(*id_by_value.fetch_values("3", "100", "20")), # 100, 3, 20 + project_with_cf_value(*id_by_value.fetch_values("20", "3", "100")), # 100, 3, 20 + project_with_cf_value(*id_by_value.fetch_values("20", "100", "3")), # 100, 3, 20 + project_with_cf_value(*id_by_value.fetch_values("100", "3", "20")), # 100, 3, 20 + project_with_cf_value(*id_by_value.fetch_values("100", "20", "3")), # 100, 3, 20 + project_with_cf_value(*id_by_value.fetch_values("20", "100")), # 100, 20 + 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 + projects.values_at(*indexes.reverse) + end end 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 f7410b6d69ce..7e2d0f1ec1f7 100644 --- a/spec/models/query/results_cf_sorting_integration_spec.rb +++ b/spec/models/query/results_cf_sorting_integration_spec.rb @@ -204,16 +204,28 @@ def wp_without_cf_value let(:work_packages) do [ - wp_with_cf_value(id_by_value.fetch_values("100")), - wp_with_cf_value(id_by_value.fetch_values("3", "100")), - wp_with_cf_value(id_by_value.fetch_values("100", "3", "20")), - wp_with_cf_value(id_by_value.fetch_values("20", "100")), - wp_with_cf_value(id_by_value.fetch_values("3")), - wp_with_cf_value(id_by_value.fetch_values("3", "20")), - wp_with_cf_value(id_by_value.fetch_values("20")), + 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 + wp_with_cf_value(id_by_value.fetch_values("3", "100", "20")), # 100, 3, 20 + wp_with_cf_value(id_by_value.fetch_values("20", "3", "100")), # 100, 3, 20 + wp_with_cf_value(id_by_value.fetch_values("20", "100", "3")), # 100, 3, 20 + wp_with_cf_value(id_by_value.fetch_values("100", "3", "20")), # 100, 3, 20 + wp_with_cf_value(id_by_value.fetch_values("100", "20", "3")), # 100, 3, 20 + wp_with_cf_value(id_by_value.fetch_values("20", "100")), # 100, 20 + 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 + work_packages.values_at(*indexes.reverse) + end end end end From 1d0a38033d513e4aead85f252e68f568b22d5af8 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 19 Sep 2024 20:07:31 +0200 Subject: [PATCH 10/16] remove duplication in list custom field ordering --- app/models/custom_field/order_statements.rb | 35 ++++++++------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 013e57d67014..1ecb59d0d3fb 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -89,28 +89,19 @@ def select_custom_value_as_string end def select_custom_option_position - if multi_value? - <<-SQL.squish - ( - SELECT array_agg(co_sort.position ORDER BY co_sort.position) - FROM #{CustomOption.quoted_table_name} co_sort - LEFT JOIN #{CustomValue.quoted_table_name} cv_sort - ON cv_sort.value IS NOT NULL AND co_sort.id = cv_sort.value::bigint - WHERE #{cv_sort_only_custom_field_condition_sql} - ) - SQL - else - <<-SQL.squish - ( - SELECT co_sort.position - FROM #{CustomOption.quoted_table_name} co_sort - LEFT JOIN #{CustomValue.quoted_table_name} cv_sort - ON cv_sort.value IS NOT NULL AND co_sort.id = cv_sort.value::bigint - WHERE #{cv_sort_only_custom_field_condition_sql} - LIMIT 1 - ) - SQL - end + 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 + LEFT JOIN #{CustomValue.quoted_table_name} cv_sort + ON cv_sort.value IS NOT NULL AND co_sort.id = cv_sort.value::bigint + WHERE #{cv_sort_only_custom_field_condition_sql} + #{limit} + ) + SQL end def select_custom_values_as_group From b6ab0231a8cf5532702400ddd51148f67698148c Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 20 Sep 2024 14:35:13 +0200 Subject: [PATCH 11/16] rename method for ordering list --- app/models/custom_field/order_statements.rb | 34 ++++++++++----------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 1ecb59d0d3fb..d5ebe139b766 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -33,7 +33,7 @@ module CustomField::OrderStatements def order_statements case field_format when "list" - [select_custom_option_position] + [order_by_list_sql] when "string", "date", "bool", "link" [coalesce_select_custom_value_as_string] when "int", "float" @@ -88,22 +88,6 @@ def select_custom_value_as_string SQL end - def select_custom_option_position - 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 - LEFT JOIN #{CustomValue.quoted_table_name} cv_sort - ON cv_sort.value IS NOT NULL AND co_sort.id = cv_sort.value::bigint - WHERE #{cv_sort_only_custom_field_condition_sql} - #{limit} - ) - SQL - end - def select_custom_values_as_group <<-SQL COALESCE((SELECT string_agg(cv_sort.value, '.') FROM #{CustomValue.quoted_table_name} cv_sort @@ -122,6 +106,22 @@ def select_custom_value_as_decimal 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 + LEFT JOIN #{CustomValue.quoted_table_name} cv_sort + ON cv_sort.value IS NOT NULL AND co_sort.id = cv_sort.value::bigint + WHERE #{cv_sort_only_custom_field_condition_sql} + #{limit} + ) + SQL + end + def order_by_user_sql <<-SQL (SELECT ARRAY[cv_user.lastname, cv_user.firstname, cv_user.mail] FROM #{User.quoted_table_name} cv_user From e58a168119fe482e1e54baf7d2b1a5d8d1c67ecf Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 20 Sep 2024 14:35:42 +0200 Subject: [PATCH 12/16] use inner join --- 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 d5ebe139b766..0ccb1c78af4b 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -114,7 +114,7 @@ def order_by_list_sql ( SELECT #{columns} FROM #{CustomOption.quoted_table_name} co_sort - LEFT JOIN #{CustomValue.quoted_table_name} cv_sort + INNER JOIN #{CustomValue.quoted_table_name} cv_sort ON cv_sort.value IS NOT NULL AND co_sort.id = cv_sort.value::bigint WHERE #{cv_sort_only_custom_field_condition_sql} #{limit} From 15db96d27327d59ceec4a362346a5d147bec9a73 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 20 Sep 2024 14:38:50 +0200 Subject: [PATCH 13/16] exclude empty values for list --- 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 0ccb1c78af4b..54a0fe232d5f 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -115,7 +115,7 @@ def order_by_list_sql 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 co_sort.id = cv_sort.value::bigint + 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} ) From 40af26bbe71cce3d1b551a193b967440b3c5e84d Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 20 Sep 2024 15:08:00 +0200 Subject: [PATCH 14/16] handle ordering of multi value user custom fields --- app/models/custom_field/order_statements.rb | 18 ++++++++++--- .../project_query_custom_field_order_spec.rb | 27 +++++++++++-------- .../results_cf_sorting_integration_spec.rb | 27 +++++++++++-------- 3 files changed, 46 insertions(+), 26 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 54a0fe232d5f..939ad6f57acf 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -123,10 +123,20 @@ def order_by_list_sql end def order_by_user_sql - <<-SQL - (SELECT ARRAY[cv_user.lastname, cv_user.firstname, cv_user.mail] FROM #{User.quoted_table_name} cv_user - WHERE cv_user.id = #{select_custom_value_as_decimal} - LIMIT 1) + 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 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 e47e4fa9f05f..5311586c1768 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 @@ -222,10 +222,10 @@ def project_without_cf_value context "for user format" do shared_let(:users) do [ - create(:user, lastname: "B", firstname: "B", login: "bb1"), - create(:user, lastname: "B", firstname: "B", login: "bb2"), - create(:user, lastname: "B", firstname: "A", login: "ba"), - create(:user, lastname: "A", firstname: "X", login: "ax") + create(:user, lastname: "B", firstname: "B", login: "bb1", mail: "bb1@o.p"), + create(:user, lastname: "B", firstname: "B", login: "bb2", mail: "bb2@o.p"), + create(:user, lastname: "B", firstname: "A", login: "ba", mail: "ba@o.p"), + create(:user, lastname: "A", firstname: "X", login: "ax", mail: "ax@o.p") ] end shared_let(:id_by_login) { users.to_h { [_1.login, _1.id] } } @@ -272,11 +272,12 @@ def project_without_cf_value let(:custom_field_values) do [ - id_by_login.fetch_values("ax"), - id_by_login.fetch_values("ba"), - # TODO: second user is ignored - id_by_login.fetch_values("bb1", "ba"), - id_by_login.fetch_values("bb1", "ax"), + 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 @@ -287,8 +288,12 @@ def project_without_cf_value end end - # TODO: second user is ignored, so order due to falling back on id asc - let(:projects_desc) { projects.values_at(4, 2, 3, 1, 0) } + 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 + projects.values_at(*indexes.reverse) + end end 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 7e2d0f1ec1f7..9edbc8045e2f 100644 --- a/spec/models/query/results_cf_sorting_integration_spec.rb +++ b/spec/models/query/results_cf_sorting_integration_spec.rb @@ -233,10 +233,10 @@ def wp_without_cf_value context "for user format" do shared_let(:users) do [ - create(:user, lastname: "B", firstname: "B", login: "bb1"), - create(:user, lastname: "B", firstname: "B", login: "bb2"), - create(:user, lastname: "B", firstname: "A", login: "ba"), - create(:user, lastname: "A", firstname: "X", login: "ax") + create(:user, lastname: "B", firstname: "B", login: "bb1", mail: "bb1@o.p"), + create(:user, lastname: "B", firstname: "B", login: "bb2", mail: "bb2@o.p"), + create(:user, lastname: "B", firstname: "A", login: "ba", mail: "ba@o.p"), + create(:user, lastname: "A", firstname: "X", login: "ax", mail: "ax@o.p") ] end shared_let(:id_by_login) { users.to_h { [_1.login, _1.id] } } @@ -271,17 +271,22 @@ def wp_without_cf_value let(:work_packages) do [ - wp_with_cf_value(id_by_login.fetch_values("ax")), - wp_with_cf_value(id_by_login.fetch_values("ba")), - # TODO: second user is ignored - wp_with_cf_value(id_by_login.fetch_values("bb1", "ba")), - wp_with_cf_value(id_by_login.fetch_values("bb1", "ax")), + 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 - # TODO: second user is ignored, so order due to falling back on id asc - let(:work_packages_desc) { work_packages.values_at(4, 2, 3, 1, 0) } + 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 + work_packages.values_at(*indexes.reverse) + end end end end From 6199be82e31770154c7faf38deafd51b46e74e68 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 20 Sep 2024 15:24:56 +0200 Subject: [PATCH 15/16] handle ordering of multi value version custom fields --- app/models/custom_field/order_statements.rb | 16 ++++++++++++---- .../project_query_custom_field_order_spec.rb | 19 ++++++++++++------- .../results_cf_sorting_integration_spec.rb | 19 ++++++++++++------- 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 939ad6f57acf..5181381b6b05 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -141,10 +141,18 @@ def order_by_user_sql end def order_by_version_sql - <<-SQL - (SELECT cv_version.name FROM #{Version.quoted_table_name} cv_version - WHERE cv_version.id = #{select_custom_value_as_decimal} - LIMIT 1) + 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 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 5311586c1768..fd13984ed995 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 @@ -332,17 +332,22 @@ def project_without_cf_value let(:projects) do [ - project_with_cf_value(*id_by_name.fetch_values("10.10.10")), - project_with_cf_value(*id_by_name.fetch_values("10.10.2")), - # TODO: second version is ignored - project_with_cf_value(*id_by_name.fetch_values("9", "10.10.2")), - project_with_cf_value(*id_by_name.fetch_values("9", "10.10.10")), + project_with_cf_value(*id_by_name.fetch_values("10.10.10")), # 10.10.10 + project_with_cf_value(*id_by_name.fetch_values("9", "10.10.10")), # 10.10.10, 9 + project_with_cf_value(*id_by_name.fetch_values("10.10.10", "9")), # 10.10.10, 9 + 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.2", "10.10.2")), # 10.10.2, 10.2 + project_with_cf_value(*id_by_name.fetch_values("10.10.2", "9")), # 10.10.2, 9 project # TODO: should be at index 0 ] end - # TODO: second version is ignored, so order due to falling back on id asc - let(:projects_desc) { projects.values_at(4, 2, 3, 1, 0) } + 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 + projects.values_at(*indexes.reverse) + end end 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 9edbc8045e2f..50cf643c9a41 100644 --- a/spec/models/query/results_cf_sorting_integration_spec.rb +++ b/spec/models/query/results_cf_sorting_integration_spec.rb @@ -324,17 +324,22 @@ def wp_without_cf_value let(:work_packages) do [ - wp_with_cf_value(id_by_name.fetch_values("10.10.10")), - wp_with_cf_value(id_by_name.fetch_values("10.10.2")), - # TODO: second version is ignored - wp_with_cf_value(id_by_name.fetch_values("9", "10.10.2")), - wp_with_cf_value(id_by_name.fetch_values("9", "10.10.10")), + wp_with_cf_value(id_by_name.fetch_values("10.10.10")), # 10.10.10 + wp_with_cf_value(id_by_name.fetch_values("9", "10.10.10")), # 10.10.10, 9 + wp_with_cf_value(id_by_name.fetch_values("10.10.10", "9")), # 10.10.10, 9 + 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.2", "10.10.2")), # 10.10.2, 10.2 + wp_with_cf_value(id_by_name.fetch_values("10.10.2", "9")), # 10.10.2, 9 wp_without_cf_value # TODO: should be at index 0 ] end - # TODO: second version is ignored, so order due to falling back on id asc - let(:work_packages_desc) { work_packages.values_at(4, 2, 3, 1, 0) } + 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 + work_packages.values_at(*indexes.reverse) + end end end end From c768f8be4f18f7a7c60deab66ad0f312707f0cd7 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Fri, 20 Sep 2024 15:28:58 +0200 Subject: [PATCH 16/16] squish and reformat other sql chunks --- app/models/custom_field/order_statements.rb | 44 +++++++++++++-------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index 5181381b6b05..98c46c8c8167 100644 --- a/app/models/custom_field/order_statements.rb +++ b/app/models/custom_field/order_statements.rb @@ -75,34 +75,46 @@ def group_by_statement def coalesce_select_custom_value_as_string # COALESCE is here to make sure that blank and NULL values are sorted equally - <<-SQL + <<-SQL.squish COALESCE(#{select_custom_value_as_string}, '') SQL end def select_custom_value_as_string - <<-SQL - (SELECT cv_sort.value FROM #{CustomValue.quoted_table_name} cv_sort - WHERE #{cv_sort_only_custom_field_condition_sql} - LIMIT 1) + <<-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 - 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.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 select_custom_value_as_decimal - <<-SQL - (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.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 @@ -157,7 +169,7 @@ def order_by_version_sql end def cv_sort_only_custom_field_condition_sql - <<-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}