Merge pull request #16776 from opf/bug/57550-custom-field-with-format…

…-version-are-ordered-as-strings

Bug/57550 custom field with format version are ordered as strings

app/models/queries/work_packages/selects/property_select.rb

@@ -95,7 +95,7 @@ def caption
     },
     version: {
       association: "version",
-      sortable: [->(table_name = Version.table_name) { Version.semver_sql(table_name) }, "name"],
+      sortable: "name",
       default_order: "ASC",
       null_handling: "NULLS LAST",
       groupable: "#{WorkPackage.table_name}.version_id"

app/models/versions/scopes/order_by_semver_name.rb

@@ -32,21 +32,7 @@ module OrderBySemverName
 
     class_methods do
       def order_by_semver_name
-        reorder semver_sql, :name
-      end
-
-      # Returns an sql for ordering which:
-      # * Returns a substring from the beginning of the name up until the first alphabetical character e.g. "1.2.3 "
-      #   from "1.2.3 ABC"
-      # * Replaces all non numerical character groups in that substring by a blank, e.g "1.2.3 " to "1 2 3 "
-      # * Splits the result into an array of individual number parts, e.g. "{1, 2, 3, ''}" from "1 2 3 "
-      # * removes all empty array items, e.g. "{1, 2, 3}" from "{1, 2, 3, ''}"
-      def semver_sql(table_name = Version.table_name)
-        sql = <<~SQL
-          array_remove(regexp_split_to_array(regexp_replace(substring(#{table_name}.name from '^[^a-zA-Z]+'), '\\D+', ' ', 'g'), ' '), '')::int[]
-        SQL
-
-        Arel.sql(sql)
+        order :name
       end
     end
   end

db/migrate/20240920152544_set_versions_name_collation.rb

@@ -0,0 +1,13 @@
+class SetVersionsNameCollation < ActiveRecord::Migration[7.1]
+  def up
+    execute <<-SQL.squish
+      CREATE COLLATION IF NOT EXISTS versions_name (provider = icu, locale = 'en-u-kn-true');
+    SQL
+
+    change_column :versions, :name, :string, collation: "versions_name"
+  end
+
+  def down
+    change_column :versions, :name, :string
+  end
+end

spec/models/queries/projects/project_query_custom_field_order_spec.rb

@@ -316,10 +316,10 @@ def project_without_cf_value
 
         let(:projects) do
           [
-            project_with_cf_value(id_by_name.fetch("10.10.10")),
-            project_with_cf_value(id_by_name.fetch("10.10.2")),
-            project_with_cf_value(id_by_name.fetch("10.2")),
             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
@@ -332,12 +332,12 @@ def project_without_cf_value
 
         let(:projects) do
           [
-            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", "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.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_with_cf_value(*id_by_name.fetch_values("10.10.10")),        # 10.10.10
             project # TODO: should be at index 0
           ]
         end

spec/models/query/results_cf_sorting_integration_spec.rb

@@ -308,10 +308,10 @@ def wp_without_cf_value
 
         let(:work_packages) do
           [
-            wp_with_cf_value(id_by_name.fetch("10.10.10")),
-            wp_with_cf_value(id_by_name.fetch("10.10.2")),
-            wp_with_cf_value(id_by_name.fetch("10.2")),
             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
           ]
         end
@@ -324,12 +324,12 @@ def wp_without_cf_value
 
         let(:work_packages) do
           [
-            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", "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.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_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

spec/models/query/results_version_integration_spec.rb

@@ -42,23 +42,23 @@
 
   let(:old_version) do
     create(:version,
-           name: "1. Old version",
+           name: "4. Old version",
            project:,
            start_date: "2019-02-02",
            effective_date: "2019-02-03")
   end
 
   let(:new_version) do
     create(:version,
-           name: "1.2 New version",
+           name: "10.2 New version",
            project:,
            start_date: "2020-02-02",
            effective_date: "2020-02-03")
   end
 
   let(:no_date_version) do
     create(:version,
-           name: "1.1 No date version",
+           name: "10.1 No date version",
            project:,
            start_date: nil,
            effective_date: nil)
@@ -69,13 +69,13 @@
            subject: "No version wp",
            project:)
   end
-  let!(:newest_version_wp) do
+  let!(:new_version_wp) do
     create(:work_package,
            subject: "Newest version wp",
            version: new_version,
            project:)
   end
-  let!(:oldest_version_wp) do
+  let!(:old_version_wp) do
     create(:work_package,
            subject: "Oldest version wp",
            version: old_version,
@@ -101,7 +101,8 @@
       q.sort_criteria = sort_criteria
     end
   end
-  let(:work_packages_asc) { [oldest_version_wp, no_date_version_wp, newest_version_wp, no_version_wp] }
+  let(:work_packages_asc) { [old_version_wp, no_date_version_wp, new_version_wp, no_version_wp] }
+  let(:work_packages_desc) { [new_version_wp, no_date_version_wp, old_version_wp, no_version_wp] }
 
   before do
     login_as(user)
@@ -136,11 +137,8 @@
     let(:sort_criteria) { [["version", "desc"]] }
 
     it "returns the correctly sorted result" do
-      # null values are still sorted last
-      work_packages_order = [newest_version_wp, no_date_version_wp, oldest_version_wp, no_version_wp]
-
       expect(query_results.work_packages.pluck(:id))
-        .to match work_packages_order.map(&:id)
+        .to match work_packages_desc.map(&:id)
     end
   end
 end

spec/models/query/sort_criteria_spec.rb

@@ -113,12 +113,8 @@
       let(:sort_criteria) { [%w[version desc], %w[start_date asc]] }
 
       it "adds the order handling (and the default order by id)" do
-        sort_sql = <<~SQL
-          array_remove(regexp_split_to_array(regexp_replace(substring(versions.name from '^[^a-zA-Z]+'), '\\D+', ' ', 'g'), ' '), '')::int[]
-        SQL
-
         expect(subject)
-          .to eq [["#{sort_sql} DESC NULLS LAST", "name DESC NULLS LAST"],
+          .to eq [["name DESC NULLS LAST"],
                   ["work_packages.start_date NULLS LAST"],
                   ["work_packages.id DESC"]]
       end

spec/models/versions/scopes/order_by_semver_name_spec.rb

@@ -2,32 +2,27 @@
 
 RSpec.describe Versions::Scopes::OrderBySemverName do
   let(:project) { create(:project) }
-  let!(:version1) do
-    create(:version, name: "aaaaa 1.", project:)
-  end
-  let!(:version2) do
-    create(:version, name: "aaaaa", project:)
-  end
-  let!(:version3) do
-    create(:version, name: "1.10. aaa", project:)
-  end
-  let!(:version4) do
-    create(:version, name: "1.1. zzz", project:, start_date: Date.today, effective_date: Date.today + 1.day)
-  end
-  let!(:version5) do
-    create(:version, name: "1.2. mmm", project:, start_date: Date.today)
-  end
-  let!(:version6) do
-    create(:version, name: "1. xxxx", project:, start_date: Date.today + 5.days)
-  end
-  let!(:version7) do
-    create(:version, name: "1.1. aaa", project:)
-  end
+  let(:names) do
+    [
+      "1. xxxx",
+      "1.1. aaa",
+      "1.1. zzz",
+      "1.2. mmm",
+      "1.10. aaa",
+      "9",
+      "10.2",
+      "10.10.2",
+      "10.10.10",
+      "aaaaa",
+      "aaaaa 1."
+    ]
+  end
+  let!(:versions) { names.map { |name| create(:version, name:, project:) } }
 
-  subject { Version.order_by_semver_name }
+  subject { Version.order_by_semver_name.order(id: :desc).to_a }
 
   it "returns the versions in semver order" do
-    expect(subject.to_a)
-      .to eql [version6, version7, version4, version5, version3, version2, version1]
+    expect(subject)
+      .to eql versions
   end
 end