Skip to content

Commit

Permalink
Merge pull request #16762 from opf/bug/57577-broken-ordering-by-multi…
Browse files Browse the repository at this point in the history 
…-value-custom-fields

Bug/57577 broken ordering by multi value custom fields
  • Loading branch information
@toy
toy authored Sep 26, 2024
2 parents 048e728 + c768f8b commit 1650a29
Show file tree
Hide file tree
Showing 4 changed files with 241 additions and 119 deletions.
130 changes: 74 additions & 56 deletions app/models/custom_field/order_statements.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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

Expand Down Expand Up @@ -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
Expand Down
88 changes: 57 additions & 31 deletions spec/models/queries/projects/project_query_custom_field_order_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,21 +58,26 @@ 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

context "in descending order" do
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
Expand Down Expand Up @@ -188,28 +193,39 @@ 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

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: "[email protected]"),
create(:user, lastname: "B", firstname: "B", login: "bb2", mail: "[email protected]"),
create(:user, lastname: "B", firstname: "A", login: "ba", mail: "[email protected]"),
create(:user, lastname: "A", firstname: "X", login: "ax", mail: "[email protected]")
]
end
shared_let(:id_by_login) { users.to_h { [_1.login, _1.id] } }
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit 1650a29

Please sign in to comment.