Bug/57577 broken ordering by multi value custom fields

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

spec/models/queries/projects/project_query_custom_field_order_spec.rb

@@ -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
@@ -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] } }
@@ -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