Adding /assign_review and /unassign_review commands

This is mostly a copy of the /assign, /unassigns,
commands. One additional item is checking if a
potentially assigned user has permissions to
review the MR.

doc/user/permissions.md

@@ -75,6 +75,7 @@ The following table depicts the various user permission levels in a project.
 | Manage user-starred metrics dashboards (*7*)      | ✓       | ✓          | ✓           | ✓        | ✓      |
 | View confidential issues                          | (*2*)   | ✓          | ✓           | ✓        | ✓      |
 | Assign issues                                     |         | ✓          | ✓           | ✓        | ✓      |
+| Assign reviewers                                  |         | ✓          | ✓           | ✓        | ✓      |
 | Label issues                                      |         | ✓          | ✓           | ✓        | ✓      |
 | Set issue weight                                  |         | ✓          | ✓           | ✓        | ✓      |
 | Lock issue threads                                |         | ✓          | ✓           | ✓        | ✓      |

doc/user/project/quick_actions.md

@@ -31,6 +31,9 @@ The following quick actions are applicable to descriptions, discussions and thre
 | `/assign @user`                       | ✓     | ✓             |      | Assign one user.                                                                                                                 |
 | `/assign @user1 @user2`               | ✓     | ✓             |      | Assign multiple users. **(STARTER)**                                                                                             |
 | `/assign me`                          | ✓     | ✓             |      | Assign yourself.                                                                                                                 |
+| `/assign_reviewer @user`              |       | ✓             |      | Assign one user as a reviewer.                                                                                                                 |
+| `/assign_reviewer @user1 @user2`      |       | ✓             |      | Assign multiple users as reviewers. **(STARTER)**                                                                                             |
+| `/assign_reviewer me`                 |       | ✓             |      | Assign yourself as a reviewer.                                                                                                                 |
 | `/award :emoji:`                      | ✓     | ✓             | ✓    | Toggle emoji award.                                                                                                              |
 | `/child_epic <epic>`                  |       |               | ✓    | Add child epic to `<epic>`. The `<epic>` value should be in the format of `&epic`, `group&epic`, or a URL to an epic ([introduced in GitLab 12.0](https://gitlab.com/gitlab-org/gitlab/-/issues/7330)). **(ULTIMATE)** |
 | `/clear_weight`                       | ✓     |               |      | Clear weight. **(STARTER)**                                                                                                      |
@@ -79,7 +82,9 @@ The following quick actions are applicable to descriptions, discussions and thre
 | `/title <new title>`                  | ✓     | ✓             | ✓    | Change title.                                                                                                                  |
 | `/todo`                               | ✓     | ✓             | ✓    | Add a to-do item.                                                                                                                   |
 | `/unassign @user1 @user2`             | ✓     | ✓             |      | Remove specific assignees. **(STARTER)**                                                                                       |
-| `/unassign`                           | ✓     | ✓             |      | Remove all assignees.                                                                                                          |
+| `/unassign`                           |       | ✓             |      | Remove all assignees.                                                                                                          |
+| `/unassign_reviewer @user1 @user2`    |       | ✓             |      | Remove specific reviewers. **(STARTER)**                                                                                       |
+| `/unassign_reviewer`                  |       | ✓             |      | Remove all reviewers.                                                                                                          |
 | `/unlabel ~label1 ~label2` or `/remove_label ~label1 ~label2` | ✓     | ✓             | ✓    | Remove specified labels.                                                                         |
 | `/unlabel` or `/remove_label` | ✓     | ✓             | ✓    | Remove all labels.                                                                          |
 | `/unlock`                             | ✓     | ✓             |      | Unlock the discussions.                                                                                                              |

ee/spec/services/ee/notes/quick_actions_service_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe Notes::QuickActionsService do
   let(:project) { create(:project, group: group) }
   let(:user) { create(:user) }
   let(:assignee) { create(:user) }
+  let(:reviewer) { create(:user) }
   let(:issue) { create(:issue, project: project) }
   let(:epic) { create(:epic, group: group)}
 
@@ -267,6 +268,35 @@ RSpec.describe Notes::QuickActionsService do
     end
   end
 
+  describe '/assign_reviewer' do
+    let(:note_text) { %(/assign_reviewer @#{user.username} @#{reviewer.username}\n) }
+
+    let(:multiline_assign_reviewer_text) do
+      <<~HEREDOC
+      /assign_reviewer @#{user.username}
+      /assign_reviewer @#{reviewer.username})
+      HEREDOC
+    end
+
+    before do
+      project.add_maintainer(reviewer)
+      project.add_maintainer(user)
+    end
+
+    context 'with a merge request' do
+      let(:note) { create(:note_on_merge_request, note: note_text, project: project) }
+
+      it_behaves_like 'assigns one or more reviewers to the merge request', multiline: false do
+        let(:target) { note.noteable }
+      end
+
+      it_behaves_like 'assigns one or more reviewers to the merge request', multiline: true do
+        let(:note) { create(:note_on_merge_request, note: multiline_assign_reviewer_text, project: project) }
+        let(:target) { note.noteable }
+      end
+    end
+  end
+
   describe '/assign' do
     let(:note_text) { %(/assign @#{user.username} @#{assignee.username}\n) }
     let(:multiline_assign_note_text) { %(/assign @#{user.username}\n/assign @#{assignee.username}) }
@@ -346,6 +376,35 @@ RSpec.describe Notes::QuickActionsService do
     end
   end
 
+  describe '/unassign_reviewer' do
+    let(:note_text) { %(/unassign_reviewer @#{reviewer.username} @#{user.username}\n) }
+
+    let(:multiline_unassign_reviewer_note_text) do
+      <<~HEREDOC
+      /unassign_reviewer @#{reviewer.username}
+      /unassign_reviewer @#{user.username}
+      HEREDOC
+    end
+
+    before do
+      project.add_maintainer(user)
+      project.add_maintainer(reviewer)
+    end
+
+    context 'with a merge request' do
+      let(:note) { create(:note_on_merge_request, note: note_text, project: project) }
+
+      it_behaves_like 'unassigning one or more reviewers', multiline: false do
+        let(:target) { note.noteable }
+      end
+
+      it_behaves_like 'unassigning one or more reviewers', multiline: true do
+        let(:note) { create(:note_on_merge_request, note: multiline_unassign_reviewer_note_text, project: project) }
+        let(:target) { note.noteable }
+      end
+    end
+  end
+
   context '/promote' do
     let(:note_text) { "/promote" }
     let(:note) { create(:note_on_issue, noteable: issue, project: project, note: note_text) }

ee/spec/services/quick_actions/interpret_service_spec.rb

@@ -4,9 +4,11 @@ require 'spec_helper'
 
 RSpec.describe QuickActions::InterpretService do
   let(:current_user) { create(:user) }
+  let(:developer) { create(:user) }
+  let(:developer2) { create(:user) }
   let(:user) { create(:user) }
-  let(:user2) { create(:user) }
-  let(:user3) { create(:user) }
+  let_it_be(:user2) { create(:user) }
+  let_it_be(:user3) { create(:user) }
   let_it_be_with_refind(:group) { create(:group) }
   let_it_be_with_refind(:project) { create(:project, :repository, :public, group: group) }
   let_it_be_with_reload(:issue) { create(:issue, project: project) }
@@ -17,6 +19,7 @@ RSpec.describe QuickActions::InterpretService do
                            multiple_merge_request_assignees: true)
 
     project.add_developer(current_user)
+    project.add_developer(developer)
   end
 
   shared_examples 'quick action is unavailable' do |action|
@@ -102,6 +105,50 @@ RSpec.describe QuickActions::InterpretService do
       end
     end
 
+    context 'assign_reviewer command' do
+      context 'with a merge request' do
+        before_all do
+          project.add_developer(user2)
+          project.add_developer(user3)
+        end
+
+        let(:merge_request) { create(:merge_request, source_project: project) }
+
+        it 'fetches reviewers and populates them if content contains /assign_reviewer' do
+          merge_request.update(reviewer_ids: [user.id])
+
+          _, updates = service.execute("/assign_reviewer @#{user2.username}\n/assign_reviewer @#{user3.username}", merge_request)
+
+          expect(updates[:reviewer_ids]).to match_array([user.id, user2.id, user3.id])
+        end
+
+        context 'assign command with multiple reviewers' do
+          it 'assigns multiple reviewers while respecting previous assignments' do
+            merge_request.update(reviewer_ids: [user.id])
+
+            _, updates = service.execute("/assign_reviewer @#{user.username}\n/assign_reviewer @#{user2.username} @#{user3.username}", merge_request)
+
+            expect(updates[:reviewer_ids]).to match_array([user.id, user2.id, user3.id])
+          end
+        end
+      end
+    end
+
+    context 'unassign_reviewer command' do
+      let(:content) { '/unassign_reviewer' }
+      let(:merge_request) { create(: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
+          merge_request.update(reviewer_ids: [user.id, user2.id, user3.id])
+
+          _, updates = service.execute("/unassign_reviewer @#{user.username} @#{user2.username}", merge_request)
+
+          expect(updates[:reviewer_ids]).to match_array([user3.id])
+        end
+      end
+    end
+
     context 'unassign command' do
       let(:content) { '/unassign' }
 
@@ -1022,7 +1069,7 @@ RSpec.describe QuickActions::InterpretService do
       it 'includes only selected assignee references' do
         _, explanations = service.explain(content, issue)
 
-        expect(explanations).to eq(["Removes assignees @#{user.username} and @#{user3.username}."])
+        expect(explanations).to eq(["Removes assignees @#{user3.username} and @#{user.username}."])
       end
     end
 

ee/spec/support/shared_examples/quick_actions/merge_request/assign_reviewer_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'assigns one or more reviewers to the merge request' do |example|
+  before do
+    target.reviewers = [reviewer]
+  end
+
+  it 'adds multiple reviewers from the list' do
+    _, update_params, message = service.execute(note)
+
+    expected_message = example[:multiline] ? "Assigned @#{user.username} as reviewer. Assigned @#{reviewer.username} as reviewer." : "Assigned @#{reviewer.username} and @#{user.username} as reviewers."
+
+    expect(update_params[:reviewer_ids]).to match_array([user.id, reviewer.id])
+    expect(message).to eq(expected_message)
+    expect { service.apply_updates(update_params, note) }.not_to raise_error
+  end
+end

ee/spec/support/shared_examples/quick_actions/merge_request/unassign_reviewer_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'unassigning one or more reviewers' do |example|
+  before do
+    target.reviewers = [reviewer]
+  end
+
+  it 'removes multiple reviewers from the list' do
+    _, update_params, message = service.execute(note)
+
+    expected_message = example[:multiline] ? "Removed reviewer @#{reviewer.username}. Removed reviewer @#{user.username}." : "Removed reviewers @#{user.username} and @#{reviewer.username}."
+
+    expect(update_params[:reviewer_ids]).to match_array([])
+    expect(message).to eq(expected_message)
+    expect { service.apply_updates(update_params, note) }.not_to raise_error
+  end
+end

lib/gitlab/quick_actions/merge_request_actions.rb

@@ -149,6 +149,105 @@ module Gitlab
 
           @execution_message[:approve] = _('Approved the current merge request.')
         end
+
+        desc do
+          if quick_action_target.allows_multiple_reviewers?
+            _('Assign reviewer(s)')
+          else
+            _('Assign reviewer')
+          end
+        end
+        explanation do |users|
+          _('Assigns %{reviewer_users_sentence} as %{reviewer_text}.') % { reviewer_users_sentence: reviewer_users_sentence(users),
+                                                                           reviewer_text: 'reviewer'.pluralize(users.size) }
+        end
+        execution_message do |users = nil|
+          if users.blank?
+            _("Failed to assign a reviewer because no user was found.")
+          else
+            users = [users.first] unless quick_action_target.allows_multiple_reviewers?
+            _('Assigned %{reviewer_users_sentence} as %{reviewer_text}.') % { reviewer_users_sentence: reviewer_users_sentence(users),
+                                                                              reviewer_text: 'reviewer'.pluralize(users.size) }
+          end
+        end
+        params do
+          quick_action_target.allows_multiple_reviewers? ? '@user1 @user2' : '@user'
+        end
+        types MergeRequest
+        condition do
+          Feature.enabled?(:merge_request_reviewers, project, default_enabled: :yaml) &&
+            current_user.can?(:"admin_#{quick_action_target.to_ability_name}", project)
+        end
+        parse_params do |reviewer_param|
+          extract_users(reviewer_param)
+        end
+        command :assign_reviewer do |users|
+          next if users.empty?
+
+          if quick_action_target.allows_multiple_reviewers?
+            @updates[:reviewer_ids] ||= quick_action_target.reviewers.map(&:id)
+            @updates[:reviewer_ids] |= users.map(&:id)
+          else
+            @updates[:reviewer_ids] = [users.first.id]
+          end
+        end
+
+        desc do
+          if quick_action_target.allows_multiple_reviewers?
+            _('Remove all or specific reviewer(s)')
+          else
+            _('Remove reviewer')
+          end
+        end
+        explanation do |users = nil|
+          reviewers = reviewers_for_removal(users)
+          _("Removes %{reviewer_text} %{reviewer_references}.") %
+            { reviewer_text: 'reviewer'.pluralize(reviewers.size), reviewer_references: reviewers.map(&:to_reference).to_sentence }
+        end
+        execution_message do |users = nil|
+          reviewers = reviewers_for_removal(users)
+          _("Removed %{reviewer_text} %{reviewer_references}.") %
+            { reviewer_text: 'reviewer'.pluralize(reviewers.size), reviewer_references: reviewers.map(&:to_reference).to_sentence }
+        end
+        params do
+          quick_action_target.allows_multiple_reviewers? ? '@user1 @user2' : ''
+        end
+        types MergeRequest
+        condition do
+          quick_action_target.persisted? &&
+            Feature.enabled?(:merge_request_reviewers, project, default_enabled: :yaml) &&
+            quick_action_target.reviewers.any? &&
+            current_user.can?(:"admin_#{quick_action_target.to_ability_name}", project)
+        end
+        parse_params do |unassign_reviewer_param|
+          # When multiple users are assigned, all will be unassigned if multiple reviewers are no longer allowed
+          extract_users(unassign_reviewer_param) if quick_action_target.allows_multiple_reviewers?
+        end
+        command :unassign_reviewer do |users = nil|
+          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
+        end
+      end
+
+      def reviewer_users_sentence(users)
+        if quick_action_target.allows_multiple_reviewers?
+          users
+        else
+          [users.first]
+        end.map(&:to_reference).to_sentence
+      end
+
+      def reviewers_for_removal(users)
+        reviewers = quick_action_target.reviewers
+        if users.present? && quick_action_target.allows_multiple_reviewers?
+          users
+        else
+          reviewers
+        end
       end
 
       def merge_orchestration_service

locale/gitlab.pot

@@ -3892,6 +3892,12 @@ msgstr ""
 msgid "Assign milestone"
 msgstr ""
 
+msgid "Assign reviewer"
+msgstr ""
+
+msgid "Assign reviewer(s)"
+msgstr ""
+
 msgid "Assign some issues to this milestone."
 msgstr ""
 
@@ -3910,6 +3916,9 @@ msgstr ""
 msgid "Assigned %{assignee_users_sentence}."
 msgstr ""
 
+msgid "Assigned %{reviewer_users_sentence} as %{reviewer_text}."
+msgstr ""
+
 msgid "Assigned Issues"
 msgstr ""
 
@@ -3954,6 +3963,9 @@ msgstr ""
 msgid "Assigns %{assignee_users_sentence}."
 msgstr ""
 
+msgid "Assigns %{reviewer_users_sentence} as %{reviewer_text}."
+msgstr ""
+
 msgid "At least one approval from a code owner is required to change files matching the respective CODEOWNER rules."
 msgstr ""
 
@@ -11721,6 +11733,9 @@ msgstr ""
 msgid "Failed to apply commands."
 msgstr ""
 
+msgid "Failed to assign a reviewer because no user was found."
+msgstr ""
+
 msgid "Failed to assign a user because no user was found."
 msgstr ""
 
@@ -23315,6 +23330,9 @@ msgstr ""
 msgid "Remove all or specific label(s)"
 msgstr ""
 
+msgid "Remove all or specific reviewer(s)"
+msgstr ""
+
 msgid "Remove approver"
 msgstr ""
 
@@ -23393,6 +23411,9 @@ msgstr ""
 msgid "Remove report"
 msgstr ""
 
+msgid "Remove reviewer"
+msgstr ""
+
 msgid "Remove secondary node"
 msgstr ""
 
@@ -23429,6 +23450,9 @@ msgstr ""
 msgid "Removed %{milestone_reference} milestone."
 msgstr ""
 
+msgid "Removed %{reviewer_text} %{reviewer_references}."
+msgstr ""
+
 msgid "Removed %{type} with id %{id}"
 msgstr ""
 
@@ -23474,6 +23498,9 @@ msgstr ""
 msgid "Removes %{milestone_reference} milestone."
 msgstr ""
 
+msgid "Removes %{reviewer_text} %{reviewer_references}."
+msgstr ""
+
 msgid "Removes all labels."
 msgstr ""
 

spec/services/quick_actions/interpret_service_spec.rb

@@ -8,6 +8,7 @@ RSpec.describe QuickActions::InterpretService do
   let_it_be(:project) { public_project }
   let_it_be(:developer) { create(:user) }
   let_it_be(:developer2) { create(:user) }
+  let_it_be(:developer3) { create(:user) }
   let_it_be_with_reload(:issue) { create(:issue, project: project) }
   let(:milestone) { create(:milestone, project: project, title: '9.10') }
   let(:commit) { create(:commit, project: project) }
@@ -23,6 +24,7 @@ RSpec.describe QuickActions::InterpretService do
 
   before do
     stub_licensed_features(multiple_issue_assignees: false,
+                           multiple_merge_request_reviewers: false,
                            multiple_merge_request_assignees: false)
   end
 
@@ -665,6 +667,20 @@ RSpec.describe QuickActions::InterpretService do
       end
     end
 
+    shared_examples 'assign_reviewer command' do
+      it 'assigns a reviewer to a single user' do
+        _, updates, _ = service.execute(content, issuable)
+
+        expect(updates).to eq(reviewer_ids: [developer.id])
+      end
+
+      it 'returns the assign reviewer message' do
+        _, _, message = service.execute(content, issuable)
+
+        expect(message).to eq("Assigned #{developer.to_reference} as reviewer.")
+      end
+    end
+
     it_behaves_like 'reopen command' do
       let(:content) { '/reopen' }
       let(:issuable) { issue }
@@ -859,6 +875,110 @@ RSpec.describe QuickActions::InterpretService do
       let(:issuable) { issue }
     end
 
+    context 'when the merge_request_reviewers flag is enabled' do
+      context 'assign_reviewer command with one user' do
+        it_behaves_like 'assign_reviewer command' do
+          let(:content) { "/assign_reviewer @#{developer.username}" }
+          let(:issuable) { merge_request }
+        end
+
+        it_behaves_like 'empty command' do
+          let(:content) { "/assign_reviewer @#{developer.username}" }
+          let(:issuable) { issue }
+        end
+      end
+
+      # CE does not have multiple reviewers
+      context 'assign_reviewer command with multiple assignees' do
+        let(:issuable) { merge_request }
+
+        it 'assigns only the first reviewer to the merge request' do
+          content = "/assign_reviewer @#{developer.username} @#{developer2.username}"
+          _, updates, _ = service.execute(content, issuable)
+
+          expect(updates).to eq(reviewer_ids: [developer.id])
+        end
+      end
+
+      context 'assign_reviewer command with me alias' do
+        it_behaves_like 'assign_reviewer command' do
+          let(:content) { '/assign_reviewer me' }
+          let(:issuable) { merge_request }
+        end
+      end
+
+      context 'assign_reviewer command with me alias and whitespace' do
+        it_behaves_like 'assign_reviewer command' do
+          let(:content) { '/assign_reviewer  me ' }
+          let(:issuable) { merge_request }
+        end
+      end
+
+      it_behaves_like 'empty command', "Failed to assign a reviewer because no user was found." do
+        let(:content) { '/assign_reviewer @abcd1234' }
+        let(:issuable) { merge_request }
+      end
+
+      it_behaves_like 'empty command', "Failed to assign a reviewer because no user was found." do
+        let(:content) { '/assign_reviewer' }
+        let(:issuable) { merge_request }
+      end
+
+      describe 'assign_reviewer command' do
+        let(:content) { "/assign_reviewer @#{developer.username} do it!" }
+
+        it 'includes only the user reference' do
+          _, explanations = service.explain(content, merge_request)
+
+          expect(explanations).to eq(["Assigns @#{developer.username} as reviewer."])
+        end
+      end
+
+      describe 'unassign_reviewer command' do
+        let(:content) { '/unassign_reviewer' }
+        let(:merge_request) { create(:merge_request, reviewers: [developer]) }
+
+        it 'includes current reviewer reference' do
+          _, explanations = service.explain(content, merge_request)
+
+          expect(explanations).to eq(["Removes reviewer @#{developer.username}."])
+        end
+
+        it 'populates reviewer_ids: [] if content contains /unassign_reviewer' do
+          _, updates, _ = service.execute(content, merge_request)
+
+          expect(updates).to eq(reviewer_ids: [])
+        end
+
+        it 'returns the unassign reviewer message for all the reviewers if content contains /unassign_reviewer' do
+          merge_request.update!(reviewer_ids: [developer.id, developer2.id])
+          _, _, message = service.execute(content, merge_request)
+
+          expect(message).to eq("Removed reviewers #{developer.to_reference} and #{developer2.to_reference}.")
+        end
+      end
+    end
+
+    context 'when the merge_request_reviewers flag is disabled' do
+      before do
+        stub_feature_flags(merge_request_reviewers: false)
+      end
+
+      describe 'assign_reviewer command' do
+        it_behaves_like 'empty command' do
+          let(:content) { "/assign_reviewer @#{developer.username}" }
+          let(:issuable) { merge_request }
+        end
+      end
+
+      describe 'unassign_reviewer command' do
+        it_behaves_like 'empty command' do
+          let(:content) { "/unassign_reviewer @#{developer.username}" }
+          let(:issuable) { merge_request }
+        end
+      end
+    end
+
     context 'unassign command' do
       let(:content) { '/unassign' }