[COMMS-863] WorkPackage <-> Version direct relation cleanup (#24396)

* remove work_package relation from Version.rb model

* remove version from the API when multiple versions is enabled

add specs

* remove 'belongs_to :version' from work_package.rb

fix

* adjust work package factory to not depend on model association

* version association removal specs fix

second pass fixing specs

third pass

second pass fixing specs

third pass fixing specs

adjust spec assigning work_package.version directly

* adjust version read using wrong method

* adjust specs that depend on the version_id column

* fix in memory changes on specs

* fix usage of version on specs

* skip tests for now

* models without a direct relation still be rendered properly

* fix more specs

fix even more specs

* unskip specs and respective adjustments

* attempt to fix baseline spec

* remove legacy version filter that was using removed association

* remove commented/unused out associations

* fix issue with dangling associations between wp and versions

because the direct associations between work package and versions has been removed, triggers like dependent: :something
won't work anymore. This means we need to handle these manually.

Author: brunopagno

app/contracts/versions/delete_contract.rb

@@ -37,9 +37,13 @@ class DeleteContract < ::DeleteContract
     protected
 
     def validate_no_work_packages_attached
-      return unless model.work_packages.exists?
+      return unless work_packages_attached?
 
       errors.add(:base, :undeletable_work_packages_attached)
     end
+
+    def work_packages_attached?
+      model.work_package_versions.exists?
+    end
   end
 end

app/contracts/work_packages/base_contract.rb

@@ -47,7 +47,8 @@ class BaseContract < ::ModelContract
     attribute :priority_id
     attribute :category_id
     attribute :version_id,
-              permission: :assign_versions do
+              permission: :assign_versions,
+              writable: ->(*) { !Setting::WorkPackageMultipleVersions.active? } do
       validate_version_is_assignable
     end
     attribute :target_versions,

app/models/queries/work_packages/filter/version_filter.rb

@@ -38,13 +38,6 @@ def human_name
     WorkPackage.human_attribute_name("version")
   end
 
-  def joins
-    case operator
-    when "o", "c", "l"
-      :version
-    end
-  end
-
   def self.key
     :version_id
   end

app/models/version.rb

@@ -33,7 +33,6 @@ class Version < ApplicationRecord
   include ::Scopes::Scoped
 
   belongs_to :project
-  has_many :work_packages, dependent: :nullify
   has_many :work_package_versions, dependent: :delete_all
   has_many :targeted_work_packages,
            -> { where(work_package_versions: { kind: "target" }) },
@@ -43,6 +42,9 @@ class Version < ApplicationRecord
            through: :work_package_versions, source: :work_package
   acts_as_customizable
 
+  # manually clear association, since the has_many/belongs_to were removed
+  before_destroy :nullify_work_package_version_mirror
+
   VERSION_STATUSES = %w(open locked closed).freeze
   VERSION_SHARINGS = %w(none descendants hierarchy tree system).freeze
 
@@ -210,6 +212,10 @@ def validate_start_date_before_effective_date
     end
   end
 
+  def nullify_work_package_version_mirror
+    WorkPackage.where(version_id: id).update_all(version_id: nil)
+  end
+
   # Returns the average estimated time of assigned issues
   # or 1 if no issue has an estimated time
   # Used to weight unestimated issues in progress calculation

app/models/work_package/versions.rb

@@ -32,11 +32,6 @@ module WorkPackage::Versions
   extend ActiveSupport::Concern
 
   included do
-    # Deprecated single-version column, kept in sync with the first target
-    # version (see #update_legacy_version_field). Can be dropped once all
-    # subsystems read target_versions instead.
-    belongs_to :version, optional: true
-
     has_many :work_package_versions, dependent: :delete_all
     has_many :versions, through: :work_package_versions, source: :version
     has_many :target_versions,
@@ -145,6 +140,19 @@ def prune_unshared_version_kinds(work_package)
     end
   end
 
+  # Read-only replacement for the former +belongs_to :version+ association.
+  #
+  # Returns the work package's single target version, or nil when it has none.
+  # Raises an error if target_versions has multiple values
+  def version
+    if target_versions.size > 1
+      raise "WorkPackage##{id} has multiple target versions and cannot be " \
+            "represented as a single version. Use #target_versions instead."
+    end
+
+    target_versions.min
+  end
+
   # Versions that the work_package can be assigned to
   # A work_package can be assigned to:
   #   * any open, shared version of the project the wp belongs to
@@ -179,8 +187,8 @@ def override_observed_in_versions? = !observed_in_version_ids_replacements.nil?
   #   * actual written target_versions
   def effective_target_versions
     if target_version_ids_replacements.nil?
-      # TODO(COMMS-863)
-      return version_id_changed? ? Array(version) : target_versions
+      # TODO(COMMS-863): drop this branch once nothing writes version_id directly
+      return version_id_changed? ? Array(Version.find_by(id: version_id)) : target_versions
     end
 
     versions_by_id = Version.where(id: target_version_ids_replacements).index_by(&:id)

app/services/projects/copy/work_packages_dependent_service.rb

@@ -136,16 +136,13 @@ def copy_relations(source_wp, new_wp_id, work_packages_map)
     end
 
     def copy_work_package_attribute_overrides(source_work_package, parent_id, user_cf_ids)
-      target_version_ids = work_package_target_version_ids(source_work_package)
+      target_version_ids = mapped_version_ids(source_work_package.target_versions)
 
       {
         project: target,
         parent_id:,
-        # TODO(COMMS-863): The legacy version_id has to agree with the first target
-        # version (contract validation) until the column is dropped.
-        version_id: target_version_ids&.first,
         target_version_ids:,
-        observed_in_version_ids: work_package_observed_in_version_ids(source_work_package),
+        observed_in_version_ids: mapped_version_ids(source_work_package.observed_in_versions),
         assigned_to_id: work_package_assigned_to_id(source_work_package),
         responsible_id: work_package_responsible_id(source_work_package),
         custom_field_values: custom_value_attributes(source_work_package, user_cf_ids),
@@ -154,20 +151,10 @@ def copy_work_package_attribute_overrides(source_work_package, parent_id, user_c
       }
     end
 
-    def work_package_target_version_ids(source_work_package)
-      lookup = state.version_id_lookup
-      return if lookup.nil?
+    def mapped_version_ids(versions)
+      lookup = state.version_id_lookup || {}
 
-      # `.presence` forces return nil instead of an empty array when the source has no target
-      # versions. This skips writing target versions unnecessarily and avoid conflicts with legacy version_id
-      source_work_package.target_versions.filter_map { |v| state.version_id_lookup[v.id] }.presence
-    end
-
-    def work_package_observed_in_version_ids(source_work_package)
-      lookup = state.version_id_lookup
-      return if lookup.nil?
-
-      source_work_package.observed_in_versions.filter_map { |v| state.version_id_lookup[v.id] }.presence
+      versions.filter_map { |version| lookup[version.id] }
     end
 
     def work_package_assigned_to_id(source_work_package)

app/services/work_packages/copy_service.rb

@@ -106,11 +106,11 @@ def version_reference_attributes(work_package, writable_attributes)
     attributes = {}
 
     if writable_attributes.include?("target_versions")
-      attributes["target_version_ids"] = work_package.target_versions.pluck(:version_id).presence
+      attributes["target_version_ids"] = work_package.target_version_ids.presence
     end
 
     if writable_attributes.include?("observed_in_versions")
-      attributes["observed_in_version_ids"] = work_package.observed_in_versions.pluck(:version_id).presence
+      attributes["observed_in_version_ids"] = work_package.observed_in_version_ids.presence
     end
 
     attributes.compact

app/services/work_packages/set_attributes_service.rb

@@ -275,7 +275,7 @@ def update_project_dependent_attributes
     return unless work_package.project_id_changed? && work_package.project_id
 
     model.change_by_system do
-      set_versions_to_nil
+      clear_unassignable_versions
       reassign_category
       set_parent_to_nil
       clear_semantic_identifier
@@ -369,15 +369,6 @@ def derive_progress_values_class
     end
   end
 
-  def set_versions_to_nil
-    if work_package.version &&
-       work_package.project&.shared_versions&.exclude?(work_package.version)
-      work_package.version = nil
-    end
-
-    clear_unassignable_versions
-  end
-
   def clear_unassignable_versions
     assignable_ids = work_package.project&.shared_versions&.pluck(:id) || []
 

lib/api/v3/work_packages/schema/work_package_schema_representer.rb

@@ -305,6 +305,8 @@ def initialize(schema, self_link:, **context)
                                          },
                                          required: false
 
+          # Deprecated in favour of `targetVersions`
+          # Removed from the API if multiple_versions is enabled on the instance
           schema_with_allowed_collection :version,
                                          value_representer: Versions::VersionRepresenter,
                                          link_factory: ->(version) {
@@ -315,6 +317,7 @@ def initialize(schema, self_link:, **context)
                                          },
                                          required: false,
                                          deprecated: true,
+                                         show_if: ->(*) { !Setting::WorkPackageMultipleVersions.active? },
                                          description: -> { I18n.t("api_v3.attributes.version.deprecated") }
 
           # While multiple versions is not enabled, the field keeps the label of the

lib/api/v3/work_packages/work_package_representer.rb

@@ -577,9 +577,20 @@ def self_v3_path(*)
                             link: ::API::V3::Principals::PrincipalRepresenterFactory
                               .create_link_lambda(:assigned_to)
 
+        # Deprecated in favour of `targetVersions`
+        # Removed from the API if multiple_versions is enabled on the instance
         associated_resource :version,
                             v3_path: :version,
-                            representer: ::API::V3::Versions::VersionRepresenter
+                            representer: ::API::V3::Versions::VersionRepresenter,
+                            # representable evaluates the getter before `skip_render`, so we manually
+                            # check if we *can* render the result here before actually doing it
+                            getter: ->(*) {
+                              next if Setting::WorkPackageMultipleVersions.active?
+                              next unless embed_link?(:version) && represented.version
+
+                              ::API::V3::Versions::VersionRepresenter.create(represented.version, current_user:)
+                            },
+                            skip_render: ->(*) { Setting::WorkPackageMultipleVersions.active? }
 
         associated_resources :target_versions,
                              v3_path: :version,