diff --git a/app/services/issuable/process_assignees.rb b/app/services/issuable/process_assignees.rb
index 1ef6d3d9c42f5a8a1673afa4c371854a08acd5b7..72f727c134da7171cce1e970d90c8aa7cca606fb 100644
--- a/app/services/issuable/process_assignees.rb
+++ b/app/services/issuable/process_assignees.rb
@@ -6,11 +6,11 @@
module Issuable
class ProcessAssignees
def initialize(assignee_ids:, add_assignee_ids:, remove_assignee_ids:, existing_assignee_ids: nil, extra_assignee_ids: nil)
- @assignee_ids = assignee_ids
- @add_assignee_ids = add_assignee_ids
- @remove_assignee_ids = remove_assignee_ids
- @existing_assignee_ids = existing_assignee_ids || []
- @extra_assignee_ids = extra_assignee_ids || []
+ @assignee_ids = assignee_ids&.map(&:to_i)
+ @add_assignee_ids = add_assignee_ids&.map(&:to_i)
+ @remove_assignee_ids = remove_assignee_ids&.map(&:to_i)
+ @existing_assignee_ids = existing_assignee_ids&.map(&:to_i) || []
+ @extra_assignee_ids = extra_assignee_ids&.map(&:to_i) || []
end
def execute
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 887c9ec84f59e042be07e562e8bce0504198efdd..e24ae8f59f0dc579be701cbced19fbea24acb339 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -67,22 +67,14 @@ def filter_params(issuable)
end
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)
+ filter_assignees_using_checks(issuable, :assignee_ids)
+ filter_assignees_using_checks(issuable, :add_assignee_ids)
+ filter_assignees_using_checks(issuable, :remove_assignee_ids)
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
-
+ def filter_assignees_using_checks(issuable, id_key)
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[id_key] = params[id_key].first(1)
end
diff --git a/app/services/merge_requests/push_options_handler_service.rb b/app/services/merge_requests/push_options_handler_service.rb
index ef251f121aeaf471aca3d41e19bc571aa4a1c952..aa52349b0ee108818785e576dd8d357c2ce13bcc 100644
--- a/app/services/merge_requests/push_options_handler_service.rb
+++ b/app/services/merge_requests/push_options_handler_service.rb
@@ -104,7 +104,7 @@ def create!(branch)
merge_request = ::MergeRequests::CreateService.new(
project: project,
current_user: current_user,
- params: merge_request.attributes.merge(assignees: merge_request.assignees,
+ params: merge_request.attributes.merge(assignee_ids: merge_request.assignee_ids,
label_ids: merge_request.label_ids)
).execute
end
@@ -140,8 +140,8 @@ def base_params
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[:add_assignee_ids] = convert_to_user_ids(params.delete(:assign).keys) if params.has_key?(:assign)
+ params[:remove_assignee_ids] = convert_to_user_ids(params.delete(:unassign).keys) if params.has_key?(:unassign)
if push_options[:milestone]
milestone = Milestone.for_projects_and_groups(@project, @project.ancestors_upto)&.find_by_name(push_options[:milestone])
@@ -169,7 +169,7 @@ def create_params(branch)
params = base_params
params.merge!(
- assignees: [current_user],
+ assignee_ids: [current_user.id],
source_branch: branch,
source_project: project,
target_project: target_project
@@ -186,6 +186,12 @@ def update_params(merge_request)
base_params.merge(merge_params(merge_request.source_branch))
end
+ def convert_to_user_ids(ids_or_usernames)
+ ids, usernames = ids_or_usernames.partition { |id_or_username| id_or_username.is_a?(Numeric) || id_or_username.match?(/\A\d+\z/) }
+ ids += User.by_username(usernames).pluck(:id) unless usernames.empty? # rubocop:disable CodeReuse/ActiveRecord
+ ids
+ end
+
def collect_errors_from_merge_request(merge_request)
merge_request.errors.full_messages.each do |error|
errors << error
diff --git a/doc/user/project/push_options.md b/doc/user/project/push_options.md
index c4f1ae86b9c73f92e450dfe945c83e3fb3315aff..b31ef858d5975ff153acf55e37f593ae07d954bc 100644
--- a/doc/user/project/push_options.md
+++ b/doc/user/project/push_options.md
@@ -67,8 +67,8 @@ time as pushing changes:
| `merge_request.milestone="<milestone>"` | Set the milestone of the merge request. Ex: `git push -o merge_request.milestone="3.0"`. | [14.1](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/63960) |
| `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. Accepts username or user ID. For example, for two users: `git push -o merge_request.assign="user1" -o merge_request.assign="user2"`. | [13.10](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/25904) |
-| `merge_request.unassign="<user>"` | Remove assigned users from the merge request. Accepts username or user ID.For example, for two users: `git push -o merge_request.unassign="user1" -o merge_request.unassign="user2"`. | [13.10](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/25904) |
+| `merge_request.assign="<user>"` | Assign users to the merge request. Accepts username or user ID. For example, for two users: `git push -o merge_request.assign="user1" -o merge_request.assign="user2"`. | [13.10](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/25904), support for usernames added in [15.5](https://gitlab.com/gitlab-org/gitlab/-/issues/344276) |
+| `merge_request.unassign="<user>"` | Remove assigned users from the merge request. Accepts username or user ID.For example, for two users: `git push -o merge_request.unassign="user1" -o merge_request.unassign="user2"`. | [13.10](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/25904), support for usernames added in [15.5](https://gitlab.com/gitlab-org/gitlab/-/issues/344276) |
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:
@@ -115,13 +115,3 @@ pipeline succeeds:
```shell
git mwps origin <local-branch-name>
```
-
-## Troubleshooting
-
-## Push options for merge request assignment ignored
-
-When you push a branch to GitLab, you can use push options to assign to (`merge_request.assign="<USERNAME>"`)
-or unassign from (`merge_request.unassign="<USERNAME>"`) a user. If GitLab creates
-the merge request successfully, but fails to assign or unassign the merge request
-correctly, you can use the user ID instead. For more information, read the issue
-[Push option `merge_request.(un)assign` seems to be ignored](https://gitlab.com/gitlab-org/gitlab/-/issues/325169).
diff --git a/ee/spec/services/ee/issues/update_service_spec.rb b/ee/spec/services/ee/issues/update_service_spec.rb
index 34bc7e6dbb020a36365488b6931d7de5e9c47e32..13c661f7ac7d2d6e08a481f363db1b8688336887 100644
--- a/ee/spec/services/ee/issues/update_service_spec.rb
+++ b/ee/spec/services/ee/issues/update_service_spec.rb
@@ -409,7 +409,7 @@ def update_issue(opts)
let_it_be(:milestone) { create(:milestone, project: project) }
let_it_be(:assignee_user1) { create(:user) }
- let(:params) { { epic: epic, milestone: milestone, assignees: [assignee_user1] } }
+ let(:params) { { epic: epic, milestone: milestone, assignee_ids: [assignee_user1.id] } }
before do
project.add_guest(assignee_user1)
@@ -420,6 +420,7 @@ def update_issue(opts)
subject
issue.reload
end.to change { issue.epic }.from(nil).to(epic)
+ .and(change { issue.assignees }.from([]).to([assignee_user1]))
.and(change { issue.milestone }.from(nil).to(milestone))
.and(change(ResourceMilestoneEvent, :count).by(1))
.and(change(Note, :count).by(3))
diff --git a/spec/features/unsubscribe_links_spec.rb b/spec/features/unsubscribe_links_spec.rb
index f90b206041829bac70e9c42ccf731740035f9968..12d2f0a9bb65f305f888086e1b50e49f3070e31c 100644
--- a/spec/features/unsubscribe_links_spec.rb
+++ b/spec/features/unsubscribe_links_spec.rb
@@ -2,13 +2,14 @@
require 'spec_helper'
-RSpec.describe 'Unsubscribe links', :sidekiq_might_not_need_inline do
+RSpec.describe 'Unsubscribe links', :sidekiq_inline do
include Warden::Test::Helpers
- let(:recipient) { create(:user) }
- let(:author) { create(:user) }
- let(:project) { create(:project, :public) }
- let(:params) { { title: 'A bug!', description: 'Fix it!', assignees: [recipient] } }
+ let_it_be(:project) { create(:project, :public) }
+ let_it_be(:author) { create(:user).tap { |u| project.add_reporter(u) } }
+ let_it_be(:recipient) { create(:user) }
+
+ let(:params) { { title: 'A bug!', description: 'Fix it!', assignee_ids: [recipient.id] } }
let(:issue) { Issues::CreateService.new(project: project, current_user: author, params: params, spam_params: nil).execute[:issue] }
let(:mail) { ActionMailer::Base.deliveries.last }
diff --git a/spec/services/issuable/process_assignees_spec.rb b/spec/services/issuable/process_assignees_spec.rb
index 45d57a1772a0bca120e14f331f04c0115a06a199..9e909b6817236eea1cc794a1cc19f0a5e4968ffe 100644
--- a/spec/services/issuable/process_assignees_spec.rb
+++ b/spec/services/issuable/process_assignees_spec.rb
@@ -12,7 +12,7 @@
extra_assignee_ids: %w(2 5 12))
result = process.execute
- expect(result.sort).to eq(%w(5 7 9).sort)
+ expect(result).to contain_exactly(5, 7, 9)
end
it 'combines other ids when assignee_ids is nil' do
@@ -23,7 +23,7 @@
extra_assignee_ids: %w(2 5 12))
result = process.execute
- expect(result.sort).to eq(%w(1 2 3 5 11 12).sort)
+ expect(result).to contain_exactly(1, 2, 3, 5, 11, 12)
end
it 'combines other ids when both add_assignee_ids and remove_assignee_ids are not empty' do
@@ -34,7 +34,7 @@
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).to contain_exactly(1, 2, 3, 5, 6, 12)
end
it 'combines other ids when remove_assignee_ids is not empty' do
@@ -45,7 +45,7 @@
extra_assignee_ids: %w(2 5 12))
result = process.execute
- expect(result.sort).to eq(%w(1 2 3 5 12).sort)
+ expect(result).to contain_exactly(1, 2, 3, 5, 12)
end
it 'combines other ids when add_assignee_ids is not empty' do
@@ -56,7 +56,7 @@
extra_assignee_ids: %w(2 5 12))
result = process.execute
- expect(result.sort).to eq(%w(1 2 4 3 5 6 11 12).sort)
+ expect(result).to contain_exactly(1, 2, 4, 3, 5, 6, 11, 12)
end
it 'combines ids when existing_assignee_ids and extra_assignee_ids are omitted' do
@@ -65,7 +65,18 @@
remove_assignee_ids: %w(4 7 11))
result = process.execute
- expect(result.sort).to eq(%w(2 6).sort)
+ expect(result.sort).to eq([2, 6].sort)
+ end
+
+ it 'handles mixed string and integer arrays' do
+ process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9),
+ add_assignee_ids: [2, 4, 6],
+ remove_assignee_ids: %w(4 7 11),
+ existing_assignee_ids: [1, 3, 11],
+ extra_assignee_ids: %w(2 5 12))
+ result = process.execute
+
+ expect(result).to contain_exactly(1, 2, 3, 5, 6, 12)
end
end
end
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
index f41fb9f9a4f2bec2dbc0c87386ec55e3d74a0904..5fe4c693451a45186aa36491ab40bb047c3b0236 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -344,7 +344,7 @@
let(:opts) do
{ title: 'Title',
description: 'Description',
- assignees: [assignee] }
+ assignee_ids: [assignee.id] }
end
it 'invalidates open issues counter for assignees when issue is assigned' do
diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb
index 4102cdc101e6244507138eda880dc81a045b9798..0bc8258af428b1b3d14b0a2ee3818d9640a14272 100644
--- a/spec/services/merge_requests/create_service_spec.rb
+++ b/spec/services/merge_requests/create_service_spec.rb
@@ -102,7 +102,7 @@
description: 'please fix',
source_branch: 'feature',
target_branch: 'master',
- assignees: [user2]
+ assignee_ids: [user2.id]
}
end
diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb
index 391377ad8018ceb11183642283f27285ad5743d8..251bf6f0d9d42badf89ecec3a6558d2f26a57ab6 100644
--- a/spec/services/merge_requests/push_options_handler_service_spec.rb
+++ b/spec/services/merge_requests/push_options_handler_service_spec.rb
@@ -730,6 +730,15 @@
it_behaves_like 'with a deleted branch'
it_behaves_like 'with the project default branch'
+
+ context 'when passing in usernames' do
+ # makes sure that usernames starting with numbers aren't treated as IDs
+ let(:user2) { create(:user, username: '123user', developer_projects: [project]) }
+ let(:user3) { create(:user, username: '999user', developer_projects: [project]) }
+ let(:assigned) { { user2.username => 1, user3.username => 1 } }
+
+ it_behaves_like 'with an existing branch that has a merge request open in foss'
+ end
end
describe '`unassign` push option' do
@@ -743,6 +752,13 @@
it_behaves_like 'with a deleted branch'
it_behaves_like 'with the project default branch'
+
+ context 'when passing in usernames' do
+ let(:assigned) { { user2.username => 1, user3.username => 1 } }
+ let(:unassigned) { { user1.username => 1, user3.username => 1 } }
+
+ it_behaves_like 'with an existing branch that has a merge request open in foss'
+ end
end
describe 'multiple pushed branches' do