diff --git a/app/contracts/work_packages/base_contract.rb b/app/contracts/work_packages/base_contract.rb index 2cb0ebb6bbdd..ce049ca14e4b 100644 --- a/app/contracts/work_packages/base_contract.rb +++ b/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 diff --git a/app/models/queries/work_packages/selects/property_select.rb b/app/models/queries/work_packages/selects/property_select.rb index a1067690d56d..0557a8a792e0 100644 --- a/app/models/queries/work_packages/selects/property_select.rb +++ b/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: { diff --git a/app/models/work_package/versions.rb b/app/models/work_package/versions.rb index 6c5df559c231..52a8e96faec9 100644 --- a/app/models/work_package/versions.rb +++ b/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 diff --git a/spec/contracts/work_packages/base_contract_spec.rb b/spec/contracts/work_packages/base_contract_spec.rb index 4b25ea70a071..ae94c6d213e2 100644 --- a/spec/contracts/work_packages/base_contract_spec.rb +++ b/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 diff --git a/spec/models/query/results_version_integration_spec.rb b/spec/models/query/results_version_integration_spec.rb index 8d5823d141c8..707479bc9b72 100644 --- a/spec/models/query/results_version_integration_spec.rb +++ b/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] diff --git a/spec/models/work_package/work_package_versions_spec.rb b/spec/models/work_package/work_package_versions_spec.rb new file mode 100644 index 000000000000..8b5814d9ab41 --- /dev/null +++ b/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