diff --git a/app/models/custom_field/order_statements.rb b/app/models/custom_field/order_statements.rb index c4be1d1fc81c..98c46c8c8167 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 + [order_by_list_sql] when "string", "date", "bool", "link" [coalesce_select_custom_value_as_string] when "int", "float" @@ -46,13 +42,9 @@ 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")] + [order_by_version_sql] end end @@ -83,77 +75,103 @@ 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.table_name} cv_sort - WHERE #{cv_sort_only_custom_field_condition_sql} - LIMIT 1) - SQL - end - - def select_custom_option_position - <<-SQL - (SELECT co_sort.position FROM #{CustomOption.table_name} co_sort - LEFT JOIN #{CustomValue.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.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.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_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 - ON cv_sort.value IS NOT NULL AND co_sort.id = cv_sort.value::numeric - WHERE #{cv_sort_only_custom_field_condition_sql}), '') + 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 select_custom_value_as_decimal - <<-SQL - (SELECT CAST(cv_sort.value AS decimal(60,3)) FROM #{CustomValue.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) + 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(column) - <<-SQL - (SELECT #{column} user_cv_#{column} FROM #{User.table_name} cv_user - WHERE cv_user.id = #{select_custom_value_as_decimal} - LIMIT 1) + 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(column) - <<-SQL - (SELECT #{column} version_cv_#{column} FROM #{Version.table_name} cv_version - WHERE cv_version.id = #{select_custom_value_as_decimal} - LIMIT 1) + 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 + <<-SQL.squish 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 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..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 @@ -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 @@ -188,17 +193,28 @@ 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")), # 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 @@ -206,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] } } @@ -256,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 @@ -271,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 @@ -311,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 1f992fb74a15..50cf643c9a41 100644 --- a/spec/models/query/results_cf_sorting_integration_spec.rb +++ b/spec/models/query/results_cf_sorting_integration_spec.rb @@ -65,16 +65,22 @@ 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 } + 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_packages_desc, &work_package_attributes) end end end @@ -199,17 +204,28 @@ 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")), # 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 @@ -217,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] } } @@ -255,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_package_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 @@ -303,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_package_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 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