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

[Backport] Support multiple assignees for merge requests

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

9 files changed, 50 insertions, 86 deletions

diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 7a4ccf0d178..26132f1824a 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -34,14 +34,20 @@ class IssuableBaseService < BaseService
   end
 
   def filter_assignee(issuable)
-    return unless params[:assignee_id].present?
+    return if params[:assignee_ids].blank?
 
-    assignee_id = params[:assignee_id]
+    unless issuable.allows_multiple_assignees?
+      params[:assignee_ids] = params[:assignee_ids].first(1)
+    end
+
+    assignee_ids = params[:assignee_ids].select { |assignee_id| assignee_can_read?(issuable, assignee_id) }
 
-    if assignee_id.to_s == IssuableFinder::NONE
-      params[:assignee_id] = ""
+    if params[:assignee_ids].map(&:to_s) == [IssuableFinder::NONE]
+      params[:assignee_ids] = []
+    elsif assignee_ids.any?
+      params[:assignee_ids] = assignee_ids
     else
-      params.delete(:assignee_id) unless assignee_can_read?(issuable, assignee_id)
+      params.delete(:assignee_ids)
     end
   end
 
@@ -352,7 +358,7 @@ class IssuableBaseService < BaseService
   end
 
   def has_changes?(issuable, old_labels: [], old_assignees: [])
-    valid_attrs = [:title, :description, :assignee_id, :milestone_id, :target_branch]
+    valid_attrs = [:title, :description, :assignee_ids, :milestone_id, :target_branch]
 
     attrs_changed = valid_attrs.any? do |attr|
       issuable.previous_changes.include?(attr.to_s)
diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb
index ef08adf4f92..48ed5afbc2a 100644
--- a/app/services/issues/base_service.rb
+++ b/app/services/issues/base_service.rb
@@ -20,7 +20,7 @@ module Issues
     private
 
     def create_assignee_note(issue, old_assignees)
-      SystemNoteService.change_issue_assignees(
+      SystemNoteService.change_issuable_assignees(
         issue, issue.project, current_user, old_assignees)
     end
 
@@ -31,26 +31,6 @@ module Issues
       issue.project.execute_services(issue_data, hooks_scope)
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def filter_assignee(issuable)
-      return if params[:assignee_ids].blank?
-
-      unless issuable.allows_multiple_assignees?
-        params[:assignee_ids] = params[:assignee_ids].take(1)
-      end
-
-      assignee_ids = params[:assignee_ids].select { |assignee_id| assignee_can_read?(issuable, assignee_id) }
-
-      if params[:assignee_ids].map(&:to_s) == [IssuableFinder::NONE]
-        params[:assignee_ids] = []
-      elsif assignee_ids.any?
-        params[:assignee_ids] = assignee_ids
-      else
-        params.delete(:assignee_ids)
-      end
-    end
-    # rubocop: enable CodeReuse/ActiveRecord
-
     def update_project_counter_caches?(issue)
       super || issue.confidential_changed?
     end
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index cec5b5734c0..cb2337d29d4 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -39,7 +39,7 @@ module Issues
       if issue.assignees != old_assignees
         create_assignee_note(issue, old_assignees)
         notification_service.async.reassigned_issue(issue, current_user, old_assignees)
-        todo_service.reassigned_issue(issue, current_user, old_assignees)
+        todo_service.reassigned_issuable(issue, current_user, old_assignees)
       end
 
       if issue.previous_changes.include?('confidential')
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index 8a9e5ebb014..b8334a87f6d 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/app/services/merge_requests/base_service.rb
@@ -49,9 +49,9 @@ module MergeRequests
       MergeRequestMetricsService.new(merge_request.metrics)
     end
 
-    def create_assignee_note(merge_request)
-      SystemNoteService.change_assignee(
-        merge_request, merge_request.project, current_user, merge_request.assignee)
+    def create_assignee_note(merge_request, old_assignees)
+      SystemNoteService.change_issuable_assignees(
+        merge_request, merge_request.project, current_user, old_assignees)
     end
 
     def create_pipeline_for(merge_request, user)
diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb
index 8112c2a4299..faaa4d66726 100644
--- a/app/services/merge_requests/update_service.rb
+++ b/app/services/merge_requests/update_service.rb
@@ -24,13 +24,13 @@ module MergeRequests
       update_task_event(merge_request) || update(merge_request)
     end
 
-    # rubocop:disable Metrics/AbcSize
     def handle_changes(merge_request, options)
       old_associations = options.fetch(:old_associations, {})
       old_labels = old_associations.fetch(:labels, [])
       old_mentioned_users = old_associations.fetch(:mentioned_users, [])
+      old_assignees = old_associations.fetch(:assignees, [])
 
-      if has_changes?(merge_request, old_labels: old_labels)
+      if has_changes?(merge_request, old_labels: old_labels, old_assignees: old_assignees)
         todo_service.mark_pending_todos_as_done(merge_request, current_user)
       end
 
@@ -45,15 +45,10 @@ module MergeRequests
                                   merge_request.target_branch)
       end
 
-      if merge_request.previous_changes.include?('assignee_id')
-        reassigned_merge_request_args = [merge_request, current_user]
-
-        old_assignee_id = merge_request.previous_changes['assignee_id'].first
-        reassigned_merge_request_args << User.find(old_assignee_id) if old_assignee_id
-
-        create_assignee_note(merge_request)
-        notification_service.async.reassigned_merge_request(*reassigned_merge_request_args)
-        todo_service.reassigned_merge_request(merge_request, current_user)
+      if merge_request.assignees != old_assignees
+        create_assignee_note(merge_request, old_assignees)
+        notification_service.async.reassigned_merge_request(merge_request, current_user, old_assignees)
+        todo_service.reassigned_issuable(merge_request, current_user, old_assignees)
       end
 
       if merge_request.previous_changes.include?('target_branch') ||
@@ -81,7 +76,6 @@ module MergeRequests
         )
       end
     end
-    # rubocop:enable Metrics/AbcSize
 
     def handle_task_changes(merge_request)
       todo_service.mark_pending_todos_as_done(merge_request, current_user)
diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 56f11b31110..760962346fb 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -247,15 +247,15 @@ module NotificationRecipientService
       attr_reader :target
       attr_reader :current_user
       attr_reader :action
-      attr_reader :previous_assignee
+      attr_reader :previous_assignees
       attr_reader :skip_current_user
 
-      def initialize(target, current_user, action:, custom_action: nil, previous_assignee: nil, skip_current_user: true)
+      def initialize(target, current_user, action:, custom_action: nil, previous_assignees: nil, skip_current_user: true)
         @target = target
         @current_user = current_user
         @action = action
         @custom_action = custom_action
-        @previous_assignee = previous_assignee
+        @previous_assignees = previous_assignees
         @skip_current_user = skip_current_user
       end
 
@@ -270,11 +270,7 @@ module NotificationRecipientService
 
         # Re-assign is considered as a mention of the new assignee
         case custom_action
-        when :reassign_merge_request
-          add_recipients(previous_assignee, :mention, nil)
-          add_recipients(target.assignee, :mention, NotificationReason::ASSIGNED)
-        when :reassign_issue
-          previous_assignees = Array(previous_assignee)
+        when :reassign_merge_request, :reassign_issue
           add_recipients(previous_assignees, :mention, nil)
           add_recipients(target.assignees, :mention, NotificationReason::ASSIGNED)
         end
@@ -287,17 +283,11 @@ module NotificationRecipientService
           # receive them, too.
           add_mentions(current_user, target: target)
 
-          # Add the assigned users, if any
-          assignees = case custom_action
-                      when :new_issue
-                        target.assignees
-                      else
-                        target.assignee
-                      end
-
           # We use the `:participating` notification level in order to match existing legacy behavior as captured
           # in existing specs (notification_service_spec.rb ~ line 507)
-          add_recipients(assignees, :participating, NotificationReason::ASSIGNED) if assignees
+          if target.is_a?(Issuable)
+            add_recipients(target.assignees, :participating, NotificationReason::ASSIGNED)
+          end
 
           add_labels_subscribers
         end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 1a65561dd70..8d3b569498f 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -95,8 +95,8 @@ class NotificationService
 
   # When we reassign an issue we should send an email to:
   #
-  #  * issue old assignee if their notification level is not Disabled
-  #  * issue new assignee if their notification level is not Disabled
+  #  * issue old assignees if their notification level is not Disabled
+  #  * issue new assignees if their notification level is not Disabled
   #  * users with custom level checked with "reassign issue"
   #
   def reassigned_issue(issue, current_user, previous_assignees = [])
@@ -104,7 +104,7 @@ class NotificationService
       issue,
       current_user,
       action: "reassign",
-      previous_assignee: previous_assignees
+      previous_assignees: previous_assignees
     )
 
     previous_assignee_ids = previous_assignees.map(&:id)
@@ -140,7 +140,7 @@ class NotificationService
   # When create a merge request we should send an email to:
   #
   #  * mr author
-  #  * mr assignee if their notification level is not Disabled
+  #  * mr assignees if their notification level is not Disabled
   #  * project team members with notification level higher then Participating
   #  * watchers of the mr's labels
   #  * users with custom level checked with "new merge request"
@@ -184,23 +184,25 @@ class NotificationService
 
   # When we reassign a merge_request we should send an email to:
   #
-  #  * merge_request old assignee if their notification level is not Disabled
-  #  * merge_request assignee if their notification level is not Disabled
+  #  * merge_request old assignees if their notification level is not Disabled
+  #  * merge_request new assignees if their notification level is not Disabled
   #  * users with custom level checked with "reassign merge request"
   #
-  def reassigned_merge_request(merge_request, current_user, previous_assignee = nil)
+  def reassigned_merge_request(merge_request, current_user, previous_assignees = [])
     recipients = NotificationRecipientService.build_recipients(
       merge_request,
       current_user,
       action: "reassign",
-      previous_assignee: previous_assignee
+      previous_assignees: previous_assignees
     )
 
+    previous_assignee_ids = previous_assignees.map(&:id)
+
     recipients.each do |recipient|
       mailer.reassigned_merge_request_email(
         recipient.user.id,
         merge_request.id,
-        previous_assignee&.id,
+        previous_assignee_ids,
         current_user.id,
         recipient.reason
       ).deliver_later
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index acbbb0da929..a39ff76b798 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -69,7 +69,7 @@ module SystemNoteService
 
   # Called when the assignees of an Issue is changed or removed
   #
-  # issue - Issue object
+  # issuable - Issuable object (responds to assignees)
   # project  - Project owning noteable
   # author   - User performing the change
   # assignees - Users being assigned, or nil
@@ -85,9 +85,9 @@ module SystemNoteService
   #   "assigned to @user1 and @user2"
   #
   # Returns the created Note object
-  def change_issue_assignees(issue, project, author, old_assignees)
-    unassigned_users = old_assignees - issue.assignees
-    added_users = issue.assignees.to_a - old_assignees
+  def change_issuable_assignees(issuable, project, author, old_assignees)
+    unassigned_users = old_assignees - issuable.assignees
+    added_users = issuable.assignees.to_a - old_assignees
 
     text_parts = []
     text_parts << "assigned to #{added_users.map(&:to_reference).to_sentence}" if added_users.any?
@@ -95,7 +95,7 @@ module SystemNoteService
 
     body = text_parts.join(' and ')
 
-    create_note(NoteSummary.new(issue, project, author, body, action: 'assignee'))
+    create_note(NoteSummary.new(issuable, project, author, body, action: 'assignee'))
   end
 
   # Called when the milestone of a Noteable is changed
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index f357dc37fe7..0ea230a44a1 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -49,12 +49,12 @@ class TodoService
     todo_users.each(&:update_todos_count_cache)
   end
 
-  # When we reassign an issue we should:
+  # When we reassign an issuable we should:
   #
-  #  * create a pending todo for new assignee if issue is assigned
+  #  * create a pending todo for new assignee if issuable is assigned
   #
-  def reassigned_issue(issue, current_user, old_assignees = [])
-    create_assignment_todo(issue, current_user, old_assignees)
+  def reassigned_issuable(issuable, current_user, old_assignees = [])
+    create_assignment_todo(issuable, current_user, old_assignees)
   end
 
   # When create a merge request we should:
@@ -82,14 +82,6 @@ class TodoService
     mark_pending_todos_as_done(merge_request, current_user)
   end
 
-  # When we reassign a merge request we should:
-  #
-  #  * creates a pending todo for new assignee if merge request is assigned
-  #
-  def reassigned_merge_request(merge_request, current_user)
-    create_assignment_todo(merge_request, current_user)
-  end
-
   # When merge a merge request we should:
   #
   #  * mark all pending todos related to the target for the current user as done