Merge branch 'assignee-push-option' into 'master'

Support adding and removing assignees w/ push opts (RUN AS-IF-FOSS)

See merge request gitlab-org/gitlab!25904

app/services/issuable/process_assignees.rb

@@ -14,12 +14,13 @@ module Issuable
     end
 
     def execute
-      if assignee_ids.blank?
-        updated_new_assignees = new_assignee_ids
+      updated_new_assignees = new_assignee_ids
+
+      if add_assignee_ids.blank? && remove_assignee_ids.blank?
+        updated_new_assignees = assignee_ids if assignee_ids
+      else
         updated_new_assignees |= add_assignee_ids if add_assignee_ids
         updated_new_assignees -= remove_assignee_ids if remove_assignee_ids
-      else
-        updated_new_assignees = assignee_ids
       end
 
       updated_new_assignees.uniq

app/services/issuable_base_service.rb

@@ -29,32 +29,48 @@ class IssuableBaseService < BaseService
       params.delete(:label_ids)
       params.delete(:assignee_ids)
       params.delete(:assignee_id)
+      params.delete(:add_assignee_ids)
+      params.delete(:remove_assignee_ids)
       params.delete(:due_date)
       params.delete(:canonical_issue_id)
       params.delete(:project)
       params.delete(:discussion_locked)
     end
 
-    filter_assignee(issuable)
+    filter_assignees(issuable)
     filter_milestone
     filter_labels
   end
 
-  def filter_assignee(issuable)
-    return if params[:assignee_ids].blank?
+  def filter_assignees(issuable)
+    filter_assignees_with_key(issuable, :assignee_ids, :assignees)
+    filter_assignees_with_key(issuable, :add_assignee_ids, :add_assignees)
+    filter_assignees_with_key(issuable, :remove_assignee_ids, :remove_assignees)
+  end
+
+  def filter_assignees_with_key(issuable, id_key, key)
+    if params[key] && params[id_key].blank?
+      params[id_key] = params[key].map(&:id)
+    end
 
+    return if params[id_key].blank?
+
+    filter_assignees_using_checks(issuable, id_key)
+  end
+
+  def filter_assignees_using_checks(issuable, id_key)
     unless issuable.allows_multiple_assignees?
-      params[:assignee_ids] = params[:assignee_ids].first(1)
+      params[id_key] = params[id_key].first(1)
     end
 
-    assignee_ids = params[:assignee_ids].select { |assignee_id| user_can_read?(issuable, assignee_id) }
+    assignee_ids = params[id_key].select { |assignee_id| user_can_read?(issuable, assignee_id) }
 
-    if params[:assignee_ids].map(&:to_s) == [IssuableFinder::Params::NONE]
-      params[:assignee_ids] = []
+    if params[id_key].map(&:to_s) == [IssuableFinder::Params::NONE]
+      params[id_key] = []
     elsif assignee_ids.any?
-      params[:assignee_ids] = assignee_ids
+      params[id_key] = assignee_ids
     else
-      params.delete(:assignee_ids)
+      params.delete(id_key)
     end
   end
 
@@ -116,6 +132,15 @@ class IssuableBaseService < BaseService
     new_label_ids.uniq
   end
 
+  def process_assignee_ids(attributes, existing_assignee_ids: nil, extra_assignee_ids: [])
+    process = Issuable::ProcessAssignees.new(assignee_ids: attributes.delete(:assignee_ids),
+                                             add_assignee_ids: attributes.delete(:add_assignee_ids),
+                                             remove_assignee_ids: attributes.delete(:remove_assignee_ids),
+                                             existing_assignee_ids: existing_assignee_ids,
+                                             extra_assignee_ids: extra_assignee_ids)
+    process.execute
+  end
+
   def handle_quick_actions(issuable)
     merge_quick_actions_into_params!(issuable)
   end
@@ -145,6 +170,10 @@ class IssuableBaseService < BaseService
     params[:author] ||= current_user
     params[:label_ids] = process_label_ids(params, extra_label_ids: issuable.label_ids.to_a)
 
+    if issuable.respond_to?(:assignee_ids)
+      params[:assignee_ids] = process_assignee_ids(params, extra_assignee_ids: issuable.assignee_ids.to_a)
+    end
+
     issuable.assign_attributes(params)
 
     before_create(issuable)
@@ -191,6 +220,7 @@ class IssuableBaseService < BaseService
     old_associations = associations_before_update(issuable)
 
     assign_requested_labels(issuable)
+    assign_requested_assignees(issuable)
 
     if issuable.changed? || params.present?
       issuable.assign_attributes(params)
@@ -354,6 +384,16 @@ class IssuableBaseService < BaseService
     issuable.touch
   end
 
+  def assign_requested_assignees(issuable)
+    return if issuable.is_a?(Epic)
+
+    assignee_ids = process_assignee_ids(params, existing_assignee_ids: issuable.assignee_ids)
+    if ids_changing?(issuable.assignee_ids, assignee_ids)
+      params[:assignee_ids] = assignee_ids
+      issuable.touch
+    end
+  end
+
   # Arrays of ids are used, but we should really use sets of ids, so
   # let's have an helper to properly check if some ids are changing
   def ids_changing?(old_array, new_array)

app/services/merge_requests/build_service.rb

@@ -16,17 +16,7 @@ module MergeRequests
       merge_request.source_project = find_source_project
       merge_request.target_project = find_target_project
 
-      # Force remove the source branch?
-      merge_request.merge_params['force_remove_source_branch'] = force_remove_source_branch
-
-      # Only assign merge requests params that are allowed
-      self.params = assign_allowed_merge_params(merge_request, params)
-
-      # Filter out params that are either not allowed or invalid
-      filter_params(merge_request)
-
-      # Filter out :add_label_ids and :remove_label_ids params
-      filter_label_id_params
+      process_params
 
       merge_request.compare_commits = []
       set_merge_request_target_branch
@@ -70,21 +60,41 @@ module MergeRequests
       end
     end
 
-    def filter_label_id_params
+    def filter_id_params
       # merge_request.assign_attributes(...) below is a Rails
       # method that only work if all the params it is passed have
       # corresponding fields in the database. As there are no fields
-      # in the database for :add_label_ids and :remove_label_ids, we
+      # in the database for :add_label_ids, :remove_label_ids,
+      # :add_assignee_ids and :remove_assignee_ids, we
       # need to remove them from the params before the call to
       # merge_request.assign_attributes(...)
       #
-      # IssuableBaseService#process_label_ids takes care
+      # IssuableBaseService#process_label_ids and
+      # IssuableBaseService#process_assignee_ids take care
       # of the removal.
       params[:label_ids] = process_label_ids(params, extra_label_ids: merge_request.label_ids.to_a)
 
+      params[:assignee_ids] = process_assignee_ids(params, extra_assignee_ids: merge_request.assignee_ids.to_a)
+
       merge_request.assign_attributes(params.to_h.compact)
     end
 
+    def process_params
+      # Force remove the source branch?
+      merge_request.merge_params['force_remove_source_branch'] = force_remove_source_branch
+
+      # Only assign merge requests params that are allowed
+      self.params = assign_allowed_merge_params(merge_request, params)
+
+      # Filter out params that are either not allowed or invalid
+      filter_params(merge_request)
+
+      # Filter out the following from params:
+      #  - :add_label_ids and :remove_label_ids
+      #  - :add_assignee_ids and :remove_assignee_ids
+      filter_id_params
+    end
+
     def find_source_project
       source_project = project_from_params(:source_project)
       return source_project if source_project.present? && can?(current_user, :create_merge_request_from, source_project)

app/services/merge_requests/push_options_handler_service.rb

@@ -129,7 +129,9 @@ module MergeRequests
         target_branch: push_options[:target],
         force_remove_source_branch: push_options[:remove_source_branch],
         label: push_options[:label],
-        unlabel: push_options[:unlabel]
+        unlabel: push_options[:unlabel],
+        assign: push_options[:assign],
+        unassign: push_options[:unassign]
       }
 
       params.compact!
@@ -137,6 +139,9 @@ module MergeRequests
       params[:add_labels] = params.delete(:label).keys if params.has_key?(:label)
       params[:remove_labels] = params.delete(:unlabel).keys if params.has_key?(:unlabel)
 
+      params[:add_assignee_ids] = params.delete(:assign).keys if params.has_key?(:assign)
+      params[:remove_assignee_ids] = params.delete(:unassign).keys if params.has_key?(:unassign)
+
       params
     end
 

changelogs/unreleased/assignee-push-option.yml

+---
+title: Support adding and removing assignees w/ push opts
+merge_request: 25904
+author:
+type: added

doc/user/project/push_options.md

@@ -68,6 +68,8 @@ time as pushing changes:
 | `merge_request.description="<description>"`  | Set the description of the merge request. Ex: `git push -o merge_request.description="The description I want"`. | [12.2](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/64320)          |
 | `merge_request.label="<label>"`              | Add labels to the merge request. If the label does not exist, it is created. For example, for two labels: `git push -o merge_request.label="label1" -o merge_request.label="label2"`. | [12.3](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/31831) |
 | `merge_request.unlabel="<label>"`            | Remove labels from the merge request. For example, for two labels: `git push -o merge_request.unlabel="label1" -o merge_request.unlabel="label2"`. | [12.3](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/31831) |
+| `merge_request.assign="<user>"`              | Assign users to the merge request. For example, for two users: `git push -o merge_request.assign="user1" -o merge_request.assign="user2"`. | [12.9](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/XXXXX) |
+| `merge_request.unassign="<user>"`            | Remove assigned users from the merge request. For example, for two users: `git push -o merge_request.unassign="user1" -o merge_request.unassign="user2"`. | [12.9](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/XXXXX) |
 
 If you use a push option that requires text with spaces in it, you need to enclose it
 in quotes (`"`). You can omit the quotes if there are no spaces. Some examples:

ee/spec/services/merge_requests/push_options_handler_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::PushOptionsHandlerService do
+  include ProjectForksHelper
+
+  let_it_be(:project) { create(:project, :public, :repository) }
+  let_it_be(:user1) { create(:user, developer_projects: [project]) }
+  let_it_be(:user2) { create(:user, developer_projects: [project]) }
+  let_it_be(:user3) { create(:user, developer_projects: [project]) }
+  let_it_be(:forked_project) { fork_project(project, user1, repository: true) }
+
+  let(:service) { described_class.new(project, user1, changes, push_options) }
+  let(:source_branch) { 'fix' }
+  let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" }
+  let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" }
+  let(:error_mr_required) { "A merge_request.create push option is required to create a merge request for branch #{source_branch}" }
+
+  before do
+    stub_licensed_features(multiple_merge_request_assignees: true)
+  end
+
+  describe '`assign` push option' do
+    let(:assigned) { { user2.id => 1, user3.id => 1 } }
+    let(:unassigned) { nil }
+    let(:push_options) { { assign: assigned, unassign: unassigned } }
+
+    it_behaves_like 'with a new branch', 2
+    it_behaves_like 'with an existing branch but no open MR', 2
+    it_behaves_like 'with an existing branch that has a merge request open', 2
+  end
+
+  describe '`unassign` push option' do
+    let(:assigned) { { user2.id => 1, user3.id => 1 } }
+    let(:unassigned) { { user1.id => 1, user3.id => 1 } }
+    let(:push_options) { { assign: assigned, unassign: unassigned } }
+
+    it_behaves_like 'with a new branch', 1
+    it_behaves_like 'with an existing branch but no open MR', 1
+    it_behaves_like 'with an existing branch that has a merge request open', 1
+  end
+end

lib/gitlab/push_options.rb

@@ -5,6 +5,7 @@ module Gitlab
     VALID_OPTIONS = HashWithIndifferentAccess.new({
       merge_request: {
         keys: [
+          :assign,
           :create,
           :description,
           :label,
@@ -12,6 +13,7 @@ module Gitlab
           :remove_source_branch,
           :target,
           :title,
+          :unassign,
           :unlabel
         ]
       },
@@ -23,7 +25,9 @@ module Gitlab
     MULTI_VALUE_OPTIONS = [
       %w[ci variable],
       %w[merge_request label],
-      %w[merge_request unlabel]
+      %w[merge_request unlabel],
+      %w[merge_request assign],
+      %w[merge_request unassign]
     ].freeze
 
     NAMESPACE_ALIASES = HashWithIndifferentAccess.new({

spec/services/issuable/process_assignees_spec.rb

@@ -4,10 +4,10 @@ require 'spec_helper'
 
 RSpec.describe Issuable::ProcessAssignees do
   describe '#execute' do
-    it 'returns assignee_ids when assignee_ids are specified' do
+    it 'returns assignee_ids when add_assignee_ids and remove_assignee_ids are not specified' do
       process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9),
-                                               add_assignee_ids: %w(2 4 6),
-                                               remove_assignee_ids: %w(4 7 11),
+                                               add_assignee_ids: nil,
+                                               remove_assignee_ids: nil,
                                                existing_assignee_ids: %w(1 3 9),
                                                extra_assignee_ids: %w(2 5 12))
       result = process.execute
@@ -15,19 +15,19 @@ RSpec.describe Issuable::ProcessAssignees do
       expect(result.sort).to eq(%w(5 7 9).sort)
     end
 
-    it 'combines other ids when assignee_ids is empty' do
-      process = Issuable::ProcessAssignees.new(assignee_ids: [],
-                                               add_assignee_ids: %w(2 4 6),
-                                               remove_assignee_ids: %w(4 7 11),
+    it 'combines other ids when assignee_ids is nil' do
+      process = Issuable::ProcessAssignees.new(assignee_ids: nil,
+                                               add_assignee_ids: nil,
+                                               remove_assignee_ids: nil,
                                                existing_assignee_ids: %w(1 3 11),
                                                extra_assignee_ids: %w(2 5 12))
       result = process.execute
 
-      expect(result.sort).to eq(%w(1 2 3 5 6 12).sort)
+      expect(result.sort).to eq(%w(1 2 3 5 11 12).sort)
     end
 
-    it 'combines other ids when assignee_ids is nil' do
-      process = Issuable::ProcessAssignees.new(assignee_ids: nil,
+    it 'combines other ids when both add_assignee_ids and remove_assignee_ids are not empty' do
+      process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9),
                                                add_assignee_ids: %w(2 4 6),
                                                remove_assignee_ids: %w(4 7 11),
                                                existing_assignee_ids: %w(1 3 11),
@@ -37,8 +37,8 @@ RSpec.describe Issuable::ProcessAssignees do
       expect(result.sort).to eq(%w(1 2 3 5 6 12).sort)
     end
 
-    it 'combines other ids when assignee_ids and add_assignee_ids are nil' do
-      process = Issuable::ProcessAssignees.new(assignee_ids: nil,
+    it 'combines other ids when remove_assignee_ids is not empty' do
+      process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9),
                                                add_assignee_ids: nil,
                                                remove_assignee_ids: %w(4 7 11),
                                                existing_assignee_ids: %w(1 3 11),
@@ -48,8 +48,8 @@ RSpec.describe Issuable::ProcessAssignees do
       expect(result.sort).to eq(%w(1 2 3 5 12).sort)
     end
 
-    it 'combines other ids when assignee_ids and remove_assignee_ids are nil' do
-      process = Issuable::ProcessAssignees.new(assignee_ids: nil,
+    it 'combines other ids when add_assignee_ids is not empty' do
+      process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9),
                                                add_assignee_ids: %w(2 4 6),
                                                remove_assignee_ids: nil,
                                                existing_assignee_ids: %w(1 3 11),
@@ -59,8 +59,8 @@ RSpec.describe Issuable::ProcessAssignees do
       expect(result.sort).to eq(%w(1 2 4 3 5 6 11 12).sort)
     end
 
-    it 'combines ids when only add_assignee_ids and remove_assignee_ids are passed' do
-      process = Issuable::ProcessAssignees.new(assignee_ids: nil,
+    it 'combines ids when existing_assignee_ids and extra_assignee_ids are omitted' do
+      process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9),
                                                add_assignee_ids: %w(2 4 6),
                                                remove_assignee_ids: %w(4 7 11))
       result = process.execute

spec/support/shared_examples/services/merge_request_shared_examples.rb

@@ -73,3 +73,93 @@ RSpec.shared_examples 'merge request reviewers cache counters invalidator' do
     described_class.new(project, user, {}).execute(merge_request)
   end
 end
+
+RSpec.shared_examples_for 'a service that can create a merge request' do
+  subject(:last_mr) { MergeRequest.last }
+
+  it 'creates a merge request with the correct target branch' do
+    branch = push_options[:target] || project.default_branch
+
+    expect { service.execute }.to change { MergeRequest.count }.by(1)
+    expect(last_mr.target_branch).to eq(branch)
+  end
+
+  context 'when project has been forked', :sidekiq_might_not_need_inline do
+    let(:forked_project) { fork_project(project, user1, repository: true) }
+    let(:service) { described_class.new(forked_project, user1, changes, push_options) }
+
+    before do
+      allow(forked_project).to receive(:empty_repo?).and_return(false)
+    end
+
+    it 'sets the correct source and target project' do
+      service.execute
+
+      expect(last_mr.source_project).to eq(forked_project)
+      expect(last_mr.target_project).to eq(project)
+    end
+  end
+end
+
+RSpec.shared_examples_for 'a service that does not create a merge request' do
+  it do
+    expect { service.execute }.not_to change { MergeRequest.count }
+  end
+end
+
+# In the non-foss version of GitLab, there can be many assignees, so
+# there 'count' can be something other than 0 or 1. In the foss
+# version of GitLab, there can be only one assignee though, so 'count'
+# can only be 0 or 1.
+RSpec.shared_examples_for 'a service that can change assignees of a merge request' do |count|
+  subject(:last_mr) { MergeRequest.last }
+
+  it 'changes assignee count' do
+    service.execute
+
+    expect(last_mr.assignees.count).to eq(count)
+  end
+end
+
+RSpec.shared_examples 'with an existing branch that has a merge request open' do |count|
+  let(:changes) { existing_branch_changes }
+  let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)}
+
+  it_behaves_like 'a service that does not create a merge request'
+  it_behaves_like 'a service that can change assignees of a merge request', count
+end
+
+RSpec.shared_examples 'when coupled with the `create` push option' do |count|
+  let(:push_options) { { create: true, assign: assigned, unassign: unassigned } }
+
+  it_behaves_like 'a service that can create a merge request'
+  it_behaves_like 'a service that can change assignees of a merge request', count
+end
+
+RSpec.shared_examples 'with a new branch' do |count|
+  let(:changes) { new_branch_changes }
+
+  it_behaves_like 'a service that does not create a merge request'
+
+  it 'adds an error to the service' do
+    service.execute
+
+    expect(service.errors).to include(error_mr_required)
+  end
+
+  it_behaves_like 'when coupled with the `create` push option', count
+end
+
+RSpec.shared_examples 'with an existing branch but no open MR' do |count|
+  let(:changes) { existing_branch_changes }
+
+  it_behaves_like 'a service that does not create a merge request'
+
+  it 'adds an error to the service' do
+    service.execute
+
+    expect(service.errors).to include(error_mr_required)
+  end
+
+  it_behaves_like 'when coupled with the `create` push option', count
+end