diff --git a/ee/spec/services/quick_actions/interpret_service_spec.rb b/ee/spec/services/quick_actions/interpret_service_spec.rb index c5f46330e45f5ce2c876021ff3914f89661ec4d1..f0e82e4fa77e425fb1075ca3faf3fc867783e31c 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 addd7f098a33f0a9779e6c377a2d5d9b327a476d..5f9dbf90c05799f8a12e542075bcd33792880c14 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 e7503fbd19b88f390224545abf57648eeee4f36c..715ca663c888b3665d17cd76111466a3e43dd3b5 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' }