[COMMS-916] Pin the primary target version convention to the lowest version id

app/contracts/work_packages/base_contract.rb

@@ -449,7 +449,7 @@ def validate_version_and_target_version_not_contradict
       # target_versions override and must not be flagged here.
       return unless changed_by_user.include?("version_id")
 
-      if model.version_id != model.target_version_ids_replacements.first
+      if model.version_id != model.target_version_ids_replacements.min
         errors.add :base, :version_and_target_versions_mutually_exclusive
       end
     end

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

@@ -103,21 +103,21 @@ def caption
     version: {
       if: -> { !Setting::WorkPackageMultipleVersions.active? },
       group_by_class_name: "Version",
+      # The primary target version (the lowest version id) represents the work
+      # package, matching the version_id mirror column and the cost report's
+      # primary-version join; the sort key is that version's name.
       sortable: <<~SQL.squish,
         (SELECT LOWER(v.name)
            FROM work_package_versions wpv
            INNER JOIN versions v ON v.id = wpv.version_id
           WHERE wpv.work_package_id = work_packages.id AND wpv.kind = 'target'
-          ORDER BY LOWER(v.name), wpv.version_id
+          ORDER BY wpv.version_id
           LIMIT 1)
       SQL
       groupable: <<~SQL.squish
-        (SELECT wpv.version_id
+        (SELECT MIN(wpv.version_id)
            FROM work_package_versions wpv
-           INNER JOIN versions v ON v.id = wpv.version_id
-          WHERE wpv.work_package_id = work_packages.id AND wpv.kind = 'target'
-          ORDER BY LOWER(v.name), wpv.version_id
-          LIMIT 1)
+          WHERE wpv.work_package_id = work_packages.id AND wpv.kind = 'target')
       SQL
     },
     target_versions: {

app/models/work_package/versions.rb

@@ -40,7 +40,7 @@ module WorkPackage::Versions
     has_many :work_package_versions, dependent: :delete_all
     has_many :versions, through: :work_package_versions, source: :version
     has_many :target_versions,
-             -> { where(work_package_versions: { kind: "target" }) },
+             -> { where(work_package_versions: { kind: "target" }).order(:id) },
              through: :work_package_versions, source: :version
     has_many :observed_in_versions,
              -> { where(work_package_versions: { kind: "observed_in" }) },
@@ -244,11 +244,12 @@ def clear_version_overrides
     system_version_overrides.clear
   end
 
-  # Keeps the deprecated single version_id column in sync with the first
-  # target version, so code still reading version_id sees a sensible value.
+  # Keeps the deprecated single version_id column in sync with the primary
+  # target version (the lowest version id, i.e. what target_versions.first
+  # reads), so code still reading version_id sees the same version.
   # Can be dropped once the version_id column is removed.
   def update_legacy_version_field
-    new_version_id = target_version_ids_replacements.first
+    new_version_id = target_version_ids_replacements.min
 
     update_columns(version_id: new_version_id) unless version_id == new_version_id
   end

spec/contracts/work_packages/base_contract_spec.rb

@@ -1377,6 +1377,34 @@
           expect(contract.errors).to be_empty
         end
       end
+
+      # With several target versions, version_id mirrors the primary target
+      # version (the lowest id), so agreement means naming that one - in any
+      # assignment order.
+      context "with multiple target versions enabled",
+              with_flag: { work_package_multiple_versions: true },
+              with_settings: { work_package_multiple_versions: true } do
+        let(:lower_version) { [assignable_version, other_assignable_version].min_by(&:id) }
+        let(:higher_version) { [assignable_version, other_assignable_version].max_by(&:id) }
+
+        it "is valid when version names the lowest target version" do
+          work_package.version = lower_version
+          work_package.target_version_ids_replacements = [higher_version.id, lower_version.id]
+          contract.validate
+
+          expect(contract.errors.symbols_for(:base))
+            .not_to include(:version_and_target_versions_mutually_exclusive)
+        end
+
+        it "is invalid when version names a non-lowest target version" do
+          work_package.version = higher_version
+          work_package.target_version_ids_replacements = [higher_version.id, lower_version.id]
+          contract.validate
+
+          expect(contract.errors.symbols_for(:base))
+            .to include(:version_and_target_versions_mutually_exclusive)
+        end
+      end
     end
 
     describe "target versions assignability" do

spec/models/query/results_version_integration_spec.rb

@@ -166,20 +166,21 @@
     end
 
     describe "sorting ASC by version" do
-      it "sorts the work package by its first target version by name" do
-        # multi_version_wp sorts by old_version ("4. Old version"), tied with
-        # old_version_wp; the tie is broken by the default id DESC criterion.
+      it "sorts the work package by its primary target version" do
+        # multi_version_wp's primary target version is new_version (the lowest
+        # version id), tying it with new_version_wp; the tie is broken by the
+        # default id DESC criterion.
         expect(query_results.work_packages.pluck(:id))
-          .to eq [multi_version_wp, old_version_wp, no_date_version_wp, new_version_wp, no_version_wp].map(&:id)
+          .to eq [old_version_wp, no_date_version_wp, multi_version_wp, new_version_wp, no_version_wp].map(&:id)
       end
     end
 
     describe "grouping by version" do
       let(:group_by) { "version" }
 
-      it "counts the work package under its first target version by name" do
+      it "counts the work package under its primary target version" do
         expect(query_results.work_package_count_by_group)
-          .to eql(old_version => 2, no_date_version => 1, new_version => 1, nil => 1)
+          .to eql(old_version => 1, no_date_version => 1, new_version => 2, nil => 1)
 
         expect(query_results.work_package_count_by_group.keys)
           .to eql [old_version, no_date_version, new_version, nil]

spec/models/work_package/work_package_versions_spec.rb

@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+
+#-- 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.
+#++
+
+require "spec_helper"
+
+RSpec.describe WorkPackage, "legacy version_id mirror" do
+  let(:project) { create(:project) }
+  let!(:lower_version) { create(:version, project:) }
+  let!(:higher_version) { create(:version, project:) }
+  let(:work_package) { create(:work_package, project:) }
+
+  # The interim primary version is `target_versions.first`, which reads the
+  # lowest version id. The mirror column must agree regardless of the order
+  # the target versions were assigned in.
+  it "mirrors the lowest target version id regardless of assignment order" do
+    work_package.target_version_ids_replacements = [higher_version.id, lower_version.id]
+    work_package.save!
+
+    expect(work_package.reload.version_id).to eq(lower_version.id)
+    expect(work_package.version_id).to eq(work_package.target_versions.first.id)
+  end
+
+  it "returns target versions in id order even when preloaded" do
+    work_package.target_version_ids_replacements = [higher_version.id, lower_version.id]
+    work_package.save!
+
+    preloaded = described_class.where(id: work_package.id).includes(:target_versions).first
+    expect(preloaded.target_versions.map(&:id)).to eq([lower_version.id, higher_version.id])
+  end
+
+  it "clears the mirror when all target versions are removed" do
+    work_package.target_version_ids_replacements = [lower_version.id]
+    work_package.save!
+
+    work_package.target_version_ids_replacements = []
+    work_package.save!
+
+    expect(work_package.reload.version_id).to be_nil
+  end
+end