Merge branch 'osw-multi-assignees-merge-requests' into 'master'

[Backport] Support multiple assignees for merge requests

See merge request gitlab-org/gitlab-ce!27089

5 files changed, 122 insertions, 82 deletions

diff --git a/app/models/concerns/deprecated_assignee.rb b/app/models/concerns/deprecated_assignee.rb
new file mode 100644
index 00000000000..7f12ce39c96
--- /dev/null
+++ b/app/models/concerns/deprecated_assignee.rb
@@ -0,0 +1,86 @@
+# frozen_string_literal: true
+
+# This module handles backward compatibility for import/export of Merge Requests after
+# multiple assignees feature was introduced. Also, it handles the scenarios where
+# the #26496 background migration hasn't finished yet.
+# Ideally, most of this code should be removed at #59457.
+module DeprecatedAssignee
+  extend ActiveSupport::Concern
+
+  def assignee_ids=(ids)
+    nullify_deprecated_assignee
+    super
+  end
+
+  def assignees=(users)
+    nullify_deprecated_assignee
+    super
+  end
+
+  def assignee_id=(id)
+    self.assignee_ids = Array(id)
+  end
+
+  def assignee=(user)
+    self.assignees = Array(user)
+  end
+
+  def assignee
+    assignees.first
+  end
+
+  def assignee_id
+    assignee_ids.first
+  end
+
+  def assignee_ids
+    if Gitlab::Database.read_only? && pending_assignees_population?
+      return Array(deprecated_assignee_id)
+    end
+
+    update_assignees_relation
+    super
+  end
+
+  def assignees
+    if Gitlab::Database.read_only? && pending_assignees_population?
+      return User.where(id: deprecated_assignee_id)
+    end
+
+    update_assignees_relation
+    super
+  end
+
+  private
+
+  # This will make the background migration process quicker (#26496) as it'll have less
+  # assignee_id rows to look through.
+  def nullify_deprecated_assignee
+    return unless persisted? && Gitlab::Database.read_only?
+
+    update_column(:assignee_id, nil)
+  end
+
+  # This code should be removed in the clean-up phase of the
+  # background migration (#59457).
+  def pending_assignees_population?
+    persisted? && deprecated_assignee_id && merge_request_assignees.empty?
+  end
+
+  # If there's an assignee_id and no relation, it means the background
+  # migration at #26496 didn't reach this merge request yet.
+  # This code should be removed in the clean-up phase of the
+  # background migration (#59457).
+  def update_assignees_relation
+    if pending_assignees_population?
+      transaction do
+        merge_request_assignees.create!(user_id: deprecated_assignee_id, merge_request_id: id)
+        update_column(:assignee_id, nil)
+      end
+    end
+  end
+
+  def deprecated_assignee_id
+    read_attribute(:assignee_id)
+  end
+end
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 17f94b4bd9b..3232c51bfbd 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -67,13 +67,6 @@ module Issuable
              allow_nil: true,
              prefix: true
 
-    delegate :name,
-             :email,
-             :public_email,
-             to: :assignee,
-             allow_nil: true,
-             prefix: true
-
     validates :author, presence: true
     validates :title, presence: true, length: { maximum: 255 }
     validate :milestone_is_valid
@@ -88,6 +81,19 @@ module Issuable
     scope :only_opened, -> { with_state(:opened) }
     scope :closed, -> { with_state(:closed) }
 
+    # rubocop:disable GitlabSecurity/SqlInjection
+    # The `to_ability_name` method is not an user input.
+    scope :assigned, -> do
+      where("EXISTS (SELECT TRUE FROM #{to_ability_name}_assignees WHERE #{to_ability_name}_id = #{to_ability_name}s.id)")
+    end
+    scope :unassigned, -> do
+      where("NOT EXISTS (SELECT TRUE FROM #{to_ability_name}_assignees WHERE #{to_ability_name}_id = #{to_ability_name}s.id)")
+    end
+    scope :assigned_to, ->(u) do
+      where("EXISTS (SELECT TRUE FROM #{to_ability_name}_assignees WHERE user_id = ? AND #{to_ability_name}_id = #{to_ability_name}s.id)", u.id)
+    end
+    # rubocop:enable GitlabSecurity/SqlInjection
+
     scope :left_joins_milestones,    -> { joins("LEFT OUTER JOIN milestones ON #{table_name}.milestone_id = milestones.id") }
     scope :order_milestone_due_desc, -> { left_joins_milestones.reorder('milestones.due_date IS NULL, milestones.id IS NULL, milestones.due_date DESC') }
     scope :order_milestone_due_asc,  -> { left_joins_milestones.reorder('milestones.due_date IS NULL, milestones.id IS NULL, milestones.due_date ASC') }
@@ -104,6 +110,7 @@ module Issuable
 
     participant :author
     participant :notes_with_associations
+    participant :assignees
 
     strip_attributes :title
 
@@ -270,6 +277,10 @@ module Issuable
     end
   end
 
+  def assignee_or_author?(user)
+    author_id == user.id || assignees.exists?(user.id)
+  end
+
   def today?
     Date.today == created_at.to_date
   end
@@ -314,11 +325,7 @@ module Issuable
       end
 
       if old_assignees != assignees
-        if self.is_a?(Issue)
-          changes[:assignees] = [old_assignees.map(&:hook_attrs), assignees.map(&:hook_attrs)]
-        else
-          changes[:assignee] = [old_assignees&.first&.hook_attrs, assignee&.hook_attrs]
-        end
+        changes[:assignees] = [old_assignees.map(&:hook_attrs), assignees.map(&:hook_attrs)]
       end
 
       if self.respond_to?(:total_time_spent)
@@ -355,10 +362,18 @@ module Issuable
   def card_attributes
     {
       'Author'   => author.try(:name),
-      'Assignee' => assignee.try(:name)
+      'Assignee' => assignee_list
     }
   end
 
+  def assignee_list
+    assignees.map(&:name).to_sentence
+  end
+
+  def assignee_username_list
+    assignees.map(&:username).to_sentence
+  end
+
   def notes_with_associations
     # If A has_many Bs, and B has_many Cs, and you do
     # `A.includes(b: :c).each { |a| a.b.includes(:c) }`, sadly ActiveRecord
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 261935fd054..5ecd2019222 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -49,10 +49,6 @@ class Issue < ApplicationRecord
 
   scope :in_projects, ->(project_ids) { where(project_id: project_ids) }
 
-  scope :assigned, -> { where('EXISTS (SELECT TRUE FROM issue_assignees WHERE issue_id = issues.id)') }
-  scope :unassigned, -> { where('NOT EXISTS (SELECT TRUE FROM issue_assignees WHERE issue_id = issues.id)') }
-  scope :assigned_to, ->(u) { where('EXISTS (SELECT TRUE FROM issue_assignees WHERE user_id = ? AND issue_id = issues.id)', u.id)}
-
   scope :with_due_date, -> { where.not(due_date: nil) }
   scope :without_due_date, -> { where(due_date: nil) }
   scope :due_before, ->(date) { where('issues.due_date < ?', date) }
@@ -75,8 +71,6 @@ class Issue < ApplicationRecord
   attr_spammable :title, spam_title: true
   attr_spammable :description, spam_description: true
 
-  participant :assignees
-
   state_machine :state, initial: :opened do
     event :close do
       transition [:opened] => :closed
@@ -155,22 +149,6 @@ class Issue < ApplicationRecord
     Gitlab::HookData::IssueBuilder.new(self).build
   end
 
-  # Returns a Hash of attributes to be used for Twitter card metadata
-  def card_attributes
-    {
-      'Author'   => author.try(:name),
-      'Assignee' => assignee_list
-    }
-  end
-
-  def assignee_or_author?(user)
-    author_id == user.id || assignees.exists?(user.id)
-  end
-
-  def assignee_list
-    assignees.map(&:name).to_sentence
-  end
-
   # `from` argument can be a Namespace or Project.
   def to_reference(from = nil, full: false)
     reference = "#{self.class.reference_prefix}#{iid}"
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 458c57c1dc6..0a39a720766 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -16,6 +16,7 @@ class MergeRequest < ApplicationRecord
   include LabelEventable
   include ReactiveCaching
   include FromUnion
+  include DeprecatedAssignee
 
   self.reactive_cache_key = ->(model) { [model.project.id, model.iid] }
   self.reactive_cache_refresh_interval = 10.minutes
@@ -69,8 +70,7 @@ class MergeRequest < ApplicationRecord
   has_many :suggestions, through: :notes
 
   has_many :merge_request_assignees
-  # Will be deprecated at https://gitlab.com/gitlab-org/gitlab-ce/issues/59457
-  belongs_to :assignee, class_name: "User"
+  has_many :assignees, class_name: "User", through: :merge_request_assignees
 
   serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize
 
@@ -79,10 +79,6 @@ class MergeRequest < ApplicationRecord
   after_update :reload_diff_if_branch_changed
   after_save :ensure_metrics
 
-  # Required until the codebase starts using this relation for single or multiple assignees.
-  # TODO: Remove at gitlab-ee#2004 implementation.
-  after_save :refresh_merge_request_assignees, if: :assignee_id_changed?
-
   # When this attribute is true some MR validation is ignored
   # It allows us to close or modify broken merge requests
   attr_accessor :allow_broken
@@ -188,19 +184,14 @@ class MergeRequest < ApplicationRecord
   end
   scope :join_project, -> { joins(:target_project) }
   scope :references_project, -> { references(:target_project) }
-  scope :assigned, -> { where("assignee_id IS NOT NULL") }
-  scope :unassigned, -> { where("assignee_id IS NULL") }
-  scope :assigned_to, ->(u) { where(assignee_id: u.id)}
   scope :with_api_entity_associations, -> {
-    preload(:author, :assignee, :notes, :labels, :milestone, :timelogs,
+    preload(:assignees, :author, :notes, :labels, :milestone, :timelogs,
             latest_merge_request_diff: [:merge_request_diff_commits],
             metrics: [:latest_closed_by, :merged_by],
             target_project: [:route, { namespace: :route }],
             source_project: [:route, { namespace: :route }])
   }
 
-  participant :assignee
-
   after_save :keep_around_commit
 
   alias_attribute :project, :target_project
@@ -337,31 +328,6 @@ class MergeRequest < ApplicationRecord
     Gitlab::HookData::MergeRequestBuilder.new(self).build
   end
 
-  # Returns a Hash of attributes to be used for Twitter card metadata
-  def card_attributes
-    {
-      'Author'   => author.try(:name),
-      'Assignee' => assignee.try(:name)
-    }
-  end
-
-  # These method are needed for compatibility with issues to not mess view and other code
-  def assignees
-    Array(assignee)
-  end
-
-  def assignee_ids
-    Array(assignee_id)
-  end
-
-  def assignee_ids=(ids)
-    write_attribute(:assignee_id, ids.last)
-  end
-
-  def assignee_or_author?(user)
-    author_id == user.id || assignee_id == user.id
-  end
-
   # `from` argument can be a Namespace or Project.
   def to_reference(from = nil, full: false)
     reference = "#{self.class.reference_prefix}#{iid}"
@@ -682,15 +648,6 @@ class MergeRequest < ApplicationRecord
     merge_request_diff || create_merge_request_diff
   end
 
-  def refresh_merge_request_assignees
-    transaction do
-      # Using it instead relation.delete_all in order to avoid adding a
-      # dependent: :delete_all (we already have foreign key cascade deletion).
-      MergeRequestAssignee.where(merge_request_id: self).delete_all
-      merge_request_assignees.create(user_id: assignee_id) if assignee_id
-    end
-  end
-
   def create_merge_request_diff
     fetch_ref!
 
@@ -1208,7 +1165,7 @@ class MergeRequest < ApplicationRecord
       variables.append(key: 'CI_MERGE_REQUEST_PROJECT_URL', value: project.web_url)
       variables.append(key: 'CI_MERGE_REQUEST_TARGET_BRANCH_NAME', value: target_branch.to_s)
       variables.append(key: 'CI_MERGE_REQUEST_TITLE', value: title)
-      variables.append(key: 'CI_MERGE_REQUEST_ASSIGNEES', value: assignee.username) if assignee
+      variables.append(key: 'CI_MERGE_REQUEST_ASSIGNEES', value: assignee_username_list) if assignees.any?
       variables.append(key: 'CI_MERGE_REQUEST_MILESTONE', value: milestone.title) if milestone
       variables.append(key: 'CI_MERGE_REQUEST_LABELS', value: label_names.join(',')) if labels.present?
       variables.concat(source_project_variables)
diff --git a/app/models/project.rb b/app/models/project.rb
index e2869fc2ad5..1493dc95fa2 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -674,6 +674,10 @@ class Project < ApplicationRecord
     { scope: :project, status: auto_devops&.enabled || Feature.enabled?(:force_autodevops_on_by_default, self) }
   end
 
+  def multiple_mr_assignees_enabled?
+    Feature.enabled?(:multiple_merge_request_assignees, self)
+  end
+
   def daily_statistics_enabled?
     Feature.enabled?(:project_daily_statistics, self, default_enabled: true)
   end