From 1942d99575ec486f650d6490f30e6d026be9b290 Mon Sep 17 00:00:00 2001
From: Jan Beckmann <king-jan1999@hotmail.de>
Date: Fri, 27 Sep 2024 02:38:49 +0000
Subject: [PATCH] Allow reviewer unassign via quick action at MR creation
When using MR templates, it can be useful to use quick
actions to unassign previously assigned reviewers.
Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/329978
Changelog: fixed
---
.../quick_actions/interpret_service_spec.rb | 7 +++++++
.../quick_actions/merge_request_actions.rb | 15 ++++++++++++--
.../quick_actions/interpret_service_spec.rb | 20 +++++++++++++++++++
3 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/ee/spec/services/quick_actions/interpret_service_spec.rb b/ee/spec/services/quick_actions/interpret_service_spec.rb
index c5f46330e45f..f0e82e4fa77e 100644
--- a/ee/spec/services/quick_actions/interpret_service_spec.rb
+++ b/ee/spec/services/quick_actions/interpret_service_spec.rb
@@ -198,6 +198,7 @@
context 'unassign_reviewer command' do
let(:content) { '/unassign_reviewer' }
let(:merge_request) { create(:merge_request, source_project: project) }
+ let(:merge_request_not_persisted) { build(:merge_request, source_project: project) }
context 'unassign_reviewer command with multiple assignees' do
it 'unassigns both reviewers if content contains /unassign_reviewer @user @user1' do
@@ -232,6 +233,12 @@
expect(updates[:reviewer_ids]).to match_array([developer2.id])
end
+
+ it 'will correctly remove the reviewer even for non-persisted merge requests' do
+ _, updates, _ = service.execute(content, merge_request_not_persisted)
+
+ expect(updates[:reviewer_ids]).to match_array([developer2.id])
+ end
end
end
end
diff --git a/lib/gitlab/quick_actions/merge_request_actions.rb b/lib/gitlab/quick_actions/merge_request_actions.rb
index addd7f098a33..5f9dbf90c057 100644
--- a/lib/gitlab/quick_actions/merge_request_actions.rb
+++ b/lib/gitlab/quick_actions/merge_request_actions.rb
@@ -351,8 +351,7 @@ module MergeRequestActions
end
types MergeRequest
condition do
- quick_action_target.persisted? &&
- reviewers_to_remove?(@updates) &&
+ reviewers_to_remove?(@updates) &&
current_user.can?(:"admin_#{quick_action_target.to_ability_name}", project)
end
parse_params do |unassign_reviewer_param|
@@ -360,12 +359,24 @@ module MergeRequestActions
extract_users(unassign_reviewer_param) if quick_action_target.allows_multiple_reviewers?
end
command :unassign_reviewer, :remove_reviewer do |users = nil|
+ current_reviewers = quick_action_target.reviewers
+ # if preceding commands have been executed already, we need to use the updated reviewer_ids
+ current_reviewers = User.find(@updates[:reviewer_ids]) if @updates[:reviewer_ids].present?
+
if quick_action_target.allows_multiple_reviewers? && users&.any?
@updates[:reviewer_ids] ||= quick_action_target.reviewers.map(&:id)
@updates[:reviewer_ids] -= users.map(&:id)
else
@updates[:reviewer_ids] = []
end
+
+ removed_reviewers = current_reviewers.select { |user| @updates[:reviewer_ids].exclude?(user.id) }
+ # only generate the message here if the change would not be traceable otherwise
+ # because all reviewers have been assigned and removed immediately
+ if removed_reviewers.present? && !reviewers_to_remove?(@updates)
+ @execution_message[:unassign_reviewer] = _("Removed %{reviewer_text} %{reviewer_references}.") %
+ { reviewer_text: 'reviewer'.pluralize(removed_reviewers.size), reviewer_references: removed_reviewers.map(&:to_reference).to_sentence }
+ end
end
end
diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb
index e7503fbd19b8..715ca663c888 100644
--- a/spec/services/quick_actions/interpret_service_spec.rb
+++ b/spec/services/quick_actions/interpret_service_spec.rb
@@ -1200,6 +1200,26 @@
it_behaves_like 'failed command', 'Could not apply unassign_reviewer command.'
end
+ context 'with a not-yet-persisted merge request and a preceding assign_reviewer command' do
+ let(:content) do
+ <<-QUICKACTION
+/assign_reviewer #{developer.to_reference}
+/unassign_reviewer #{developer.to_reference}
+ QUICKACTION
+ end
+
+ let(:issuable) { build(:merge_request) }
+
+ it 'adds and then removes a single reviewer in a single step' do
+ _, updates, message = service.execute(content, issuable)
+ translated_string = _("Assigned %{developer_to_reference} as reviewer. Removed reviewer %{developer_to_reference}.")
+ formatted_message = format(translated_string, developer_to_reference: developer.to_reference.to_s)
+
+ expect(updates).to eq(reviewer_ids: [])
+ expect(message).to eq(formatted_message)
+ end
+ end
+
context 'with anything after the command' do
let(:content) { '/unassign_reviewer supercalifragilisticexpialidocious' }
--
GitLab