Add additional aliases and refactor interpret_service_spec.rb

* Slightly modifies the original commands to compensate for a plural
error as well.

lib/gitlab/quick_actions/merge_request_actions.rb

@@ -163,16 +163,17 @@ module Gitlab
           end
         end
         explanation do |users|
+          reviewers = reviewers_to_add(users)
           _('Assigns %{reviewer_users_sentence} as %{reviewer_text}.') % { reviewer_users_sentence: reviewer_users_sentence(users),
-                                                                           reviewer_text: 'reviewer'.pluralize(users.size) }
+                                                                           reviewer_text: 'reviewer'.pluralize(reviewers.size) }
         end
         execution_message do |users = nil|
-          if users.blank?
+          reviewers = reviewers_to_add(users)
+          if reviewers.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) }
+                                                                              reviewer_text: 'reviewer'.pluralize(reviewers.size) }
           end
         end
         params do
@@ -186,7 +187,7 @@ module Gitlab
         parse_params do |reviewer_param|
           extract_users(reviewer_param)
         end
-        command :assign_reviewer do |users|
+        command :assign_reviewer, :reviewer do |users|
           next if users.empty?
 
           if quick_action_target.allows_multiple_reviewers?
@@ -228,7 +229,7 @@ module Gitlab
           # 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|
+        command :unassign_reviewer, :remove_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)
@@ -239,11 +240,7 @@ module Gitlab
       end
 
       def reviewer_users_sentence(users)
-        if quick_action_target.allows_multiple_reviewers?
-          users
-        else
-          [users.first]
-        end.map(&:to_reference).to_sentence
+        reviewers_to_add(users).map(&:to_reference).to_sentence
       end
 
       def reviewers_for_removal(users)
@@ -255,6 +252,16 @@ module Gitlab
         end
       end
 
+      def reviewers_to_add(users)
+        if users.present?
+          if quick_action_target.allows_multiple_reviewers?
+            users
+          else
+            [users.first]
+          end
+        end
+      end
+
       def merge_orchestration_service
         @merge_orchestration_service ||= MergeRequests::MergeOrchestrationService.new(project, current_user)
       end

spec/services/quick_actions/interpret_service_spec.rb

@@ -669,15 +669,23 @@ RSpec.describe QuickActions::InterpretService do
 
     shared_examples 'assign_reviewer command' do
       it 'assigns a reviewer to a single user' do
-        _, updates, _ = service.execute(content, issuable)
+        _, explanations = service.explain(content, issuable)
+        _, updates, message = service.execute(content, issuable)
 
         expect(updates).to eq(reviewer_ids: [developer.id])
+        expect(message).to eq("Assigned #{developer.to_reference} as reviewer.")
+        expect(explanations).to eq(["Assigns #{developer.to_reference} as reviewer."])
       end
+    end
 
-      it 'returns the assign reviewer message' do
-        _, _, message = service.execute(content, issuable)
+    shared_examples 'unassign_reviewer command' do
+      it 'removes a single reviewer' do
+        _, explanations = service.explain(content, issuable)
+        _, updates, message = service.execute(content, issuable)
 
-        expect(message).to eq("Assigned #{developer.to_reference} as reviewer.")
+        expect(updates).to eq(reviewer_ids: [])
+        expect(message).to eq("Removed reviewer #{developer.to_reference}.")
+        expect(explanations).to eq(["Removes reviewer @#{developer.username}."])
       end
     end
 
@@ -876,85 +884,117 @@ RSpec.describe QuickActions::InterpretService do
     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 }
+      describe 'assign_reviewer command' do
+        let(:content) { "/assign_reviewer @#{developer.username}" }
+        let(:issuable) { merge_request }
+
+        context 'with one user' do
+          it_behaves_like 'assign_reviewer command'
         end
 
-        it_behaves_like 'empty command' do
-          let(:content) { "/assign_reviewer @#{developer.username}" }
+        context 'with an issue instead of a merge request' do
           let(:issuable) { issue }
+
+          it_behaves_like 'empty command'
         end
-      end
 
-      # CE does not have multiple reviewers
-      context 'assign_reviewer command with multiple assignees' do
-        let(:issuable) { merge_request }
+        # CE does not have multiple reviewers
+        context 'assign command with multiple assignees' do
+          before do
+            project.add_developer(developer2)
+          end
 
-        it 'assigns only the first reviewer to the merge request' do
-          content = "/assign_reviewer @#{developer.username} @#{developer2.username}"
-          _, updates, _ = service.execute(content, issuable)
+          # There's no guarantee that the reference extractor will preserve
+          # the order of the mentioned users since this is dependent on the
+          # order in which rows are returned. We just ensure that at least
+          # one of the mentioned users is assigned.
+          context 'assigns to one of the two users' do
+            let(:content) { "/assign_reviewer @#{developer.username} @#{developer2.username}" }
 
-          expect(updates).to eq(reviewer_ids: [developer.id])
+            it 'assigns to a single reviewer' do
+              _, updates, message = service.execute(content, issuable)
+
+              expect(updates[:reviewer_ids].count).to eq(1)
+              reviewer = updates[:reviewer_ids].first
+              expect([developer.id, developer2.id]).to include(reviewer)
+
+              user = reviewer == developer.id ? developer : developer2
+
+              expect(message).to match("Assigned #{user.to_reference} as reviewer.")
+            end
+          end
         end
-      end
 
-      context 'assign_reviewer command with me alias' do
-        it_behaves_like 'assign_reviewer command' do
+        context 'with "me" alias' do
           let(:content) { '/assign_reviewer me' }
-          let(:issuable) { merge_request }
+
+          it_behaves_like 'assign_reviewer command'
         end
-      end
 
-      context 'assign_reviewer command with me alias and whitespace' do
-        it_behaves_like 'assign_reviewer command' do
+        context 'with an alias and whitespace' do
           let(:content) { '/assign_reviewer  me ' }
-          let(:issuable) { merge_request }
+
+          it_behaves_like 'assign_reviewer command'
         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
+        context 'with an incorrect user' do
+          let(:content) { '/assign_reviewer @abcd1234' }
 
-      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
+          it_behaves_like 'empty command', "Failed to assign a reviewer because no user was found."
+        end
 
-      describe 'assign_reviewer command' do
-        let(:content) { "/assign_reviewer @#{developer.username} do it!" }
+        context 'with the "reviewer" alias' do
+          let(:content) { "/reviewer @#{developer.username}" }
+
+          it_behaves_like 'assign_reviewer command'
+        end
+
+        context 'with no user' do
+          let(:content) { '/assign_reviewer' }
+
+          it_behaves_like 'empty command', "Failed to assign a reviewer because no user was found."
+        end
 
-        it 'includes only the user reference' do
-          _, explanations = service.explain(content, merge_request)
+        context 'includes only the user reference with extra text' do
+          let(:content) { "/assign_reviewer @#{developer.username} do it!" }
 
-          expect(explanations).to eq(["Assigns @#{developer.username} as reviewer."])
+          it_behaves_like 'assign_reviewer command'
         end
       end
 
       describe 'unassign_reviewer command' do
-        let(:content) { '/unassign_reviewer' }
-        let(:merge_request) { create(:merge_request, reviewers: [developer]) }
+        # CE does not have multiple reviewers, so basically anything
+        # after /unassign_reviewer (including whitespace) will remove
+        # all the current reviewers.
+        let(:issuable) { create(:merge_request, reviewers: [developer]) }
+        let(:content) { "/unassign_reviewer @#{developer.username}" }
+
+        context 'with one user' do
+          it_behaves_like 'unassign_reviewer command'
+        end
 
-        it 'includes current reviewer reference' do
-          _, explanations = service.explain(content, merge_request)
+        context 'with an issue instead of a merge request' do
+          let(:issuable) { issue }
 
-          expect(explanations).to eq(["Removes reviewer @#{developer.username}."])
+          it_behaves_like 'empty command'
         end
 
-        it 'populates reviewer_ids: [] if content contains /unassign_reviewer' do
-          _, updates, _ = service.execute(content, merge_request)
+        context 'with anything after the command' do
+          let(:content) { '/unassign_reviewer supercalifragilisticexpialidocious' }
 
-          expect(updates).to eq(reviewer_ids: [])
+          it_behaves_like 'unassign_reviewer command'
         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)
+        context 'with the "remove_reviewer" alias' do
+          let(:content) { "/remove_reviewer @#{developer.username}" }
+
+          it_behaves_like 'unassign_reviewer command'
+        end
+
+        context 'with no user' do
+          let(:content) { '/unassign_reviewer' }
 
-          expect(message).to eq("Removed reviewers #{developer.to_reference} and #{developer2.to_reference}.")
+          it_behaves_like 'unassign_reviewer command'
         end
       end
     end