Merge branch 'ajk-quick-action-parse-errors' into 'master'

Introduce concept of parse errors for quick actions

See merge request gitlab-org/gitlab!79948

app/services/quick_actions/interpret_service.rb

@@ -61,6 +61,10 @@ module QuickActions
 
     private
 
+    def failed_parse(message)
+      raise Gitlab::QuickActions::CommandDefinition::ParseError, message
+    end
+
     def extractor
       Gitlab::QuickActions::Extractor.new(self.class.command_definitions)
     end
@@ -69,6 +73,7 @@ module QuickActions
     def extract_users(params)
       return [] if params.nil?
 
+      args = params.split(' ').uniq
       users = extract_references(params, :user)
 
       if users.empty?
@@ -76,10 +81,12 @@ module QuickActions
           if params.strip == 'me'
             [current_user]
           else
-            User.where(username: params.split(' ').map(&:strip))
+            User.where(username: args)
           end
       end
 
+      failed_parse(format(_("Failed to find users for '%{params}'"), params: params)) unless users.size == args.size
+
       users
     end
     # rubocop: enable CodeReuse/ActiveRecord

ee/spec/services/quick_actions/interpret_service_spec.rb

@@ -194,6 +194,15 @@ RSpec.describe QuickActions::InterpretService do
 
           expect(updates[:reviewer_ids]).to match_array([user3.id])
         end
+
+        it 'does not unassign reviewers if the content cannot be parsed' do
+          merge_request.update!(reviewer_ids: [user.id, user2.id, user3.id])
+
+          _, updates, msg = service.execute("/unassign_reviewer nobody", merge_request)
+
+          expect(updates[:reviewer_ids]).to be_nil
+          expect(msg).to eq "Could not apply unassign_reviewer command. Failed to find users for 'nobody'."
+        end
       end
     end
 
@@ -224,9 +233,18 @@ RSpec.describe QuickActions::InterpretService do
 
           expect(updates[:assignee_ids]).to be_empty
         end
+
+        it 'does not apply command if the argument cannot be parsed' do
+          issue.update!(assignee_ids: [user.id, user2.id])
+
+          _, updates, msg = service.execute("/assign nobody", issue)
+
+          expect(updates[:assignee_ids]).to be_nil
+          expect(msg).to eq "Could not apply assign command. Failed to find users for 'nobody'."
+        end
       end
 
-      context 'Merge Request' do
+      context 'with a Merge Request' do
         let(:merge_request) { create(:merge_request, source_project: project) }
 
         it 'unassigns user if content contains /unassign @user' do
@@ -237,7 +255,7 @@ RSpec.describe QuickActions::InterpretService do
           expect(updates[:assignee_ids]).to match_array([user.id])
         end
 
-        context 'unassign command with multiple assignees' do
+        describe 'applying unassign command with multiple assignees' do
           it 'unassigns both users if content contains /unassign @user @user1' do
             merge_request.update!(assignee_ids: [user.id, user2.id, user3.id])
 
@@ -246,7 +264,7 @@ RSpec.describe QuickActions::InterpretService do
             expect(updates[:assignee_ids]).to match_array([user3.id])
           end
 
-          context 'unlicensed' do
+          context 'when unlicensed' do
             before do
               stub_licensed_features(multiple_merge_request_assignees: false)
             end

lib/gitlab/quick_actions/command_definition.rb

@@ -3,6 +3,8 @@
 module Gitlab
   module QuickActions
     class CommandDefinition
+      ParseError = Class.new(StandardError)
+
       attr_accessor :name, :aliases, :description, :explanation, :execution_message,
         :params, :condition_block, :parse_params_block, :action_block, :warning, :icon, :types
 
@@ -41,7 +43,11 @@ module Gitlab
         return unless available?(context)
 
         message = if explanation.respond_to?(:call)
-                    execute_block(explanation, context, arg)
+                    begin
+                      execute_block(explanation, context, arg)
+                    rescue ParseError => e
+                      format(_('Problem with %{name} command: %{message}.'), name: name, message: e.message)
+                    end
                   else
                     explanation
                   end
@@ -63,6 +69,8 @@ module Gitlab
         return unless available?(context)
 
         execute_block(action_block, context, arg)
+      rescue ParseError
+        # message propagation is handled in `execution_message`.
       end
 
       def execute_message(context, arg)
@@ -74,6 +82,8 @@ module Gitlab
         else
           execution_message
         end
+      rescue ParseError => e
+        format _('Could not apply %{name} command. %{message}.'), name: name, message: e.message
       end
 
       def to_h(context)

lib/gitlab/quick_actions/merge_request_actions.rb

@@ -184,7 +184,7 @@ module Gitlab
         execution_message do |users = nil|
           reviewers = reviewers_to_add(users)
           if reviewers.blank?
-            _("Failed to assign a reviewer because no user was found.")
+            _("Failed to assign a reviewer because no user was specified.")
           else
             _('Assigned %{reviewer_users_sentence} as %{reviewer_text}.') % { reviewer_users_sentence: reviewer_users_sentence(users),
                                                                               reviewer_text: 'reviewer'.pluralize(reviewers.size) }

locale/gitlab.pot

@@ -9953,6 +9953,9 @@ msgstr ""
 msgid "Could not apply %{name} command."
 msgstr ""
 
+msgid "Could not apply %{name} command. %{message}."
+msgstr ""
+
 msgid "Could not authorize chat nickname. Try again!"
 msgstr ""
 
@@ -14789,7 +14792,7 @@ msgid_plural "Failed to archive designs. Please try again."
 msgstr[0] ""
 msgstr[1] ""
 
-msgid "Failed to assign a reviewer because no user was found."
+msgid "Failed to assign a reviewer because no user was specified."
 msgstr ""
 
 msgid "Failed to assign a user because no user was found."
@@ -14855,6 +14858,9 @@ msgstr ""
 msgid "Failed to find import label for Jira import."
 msgstr ""
 
+msgid "Failed to find users for '%{params}'"
+msgstr ""
+
 msgid "Failed to generate export, please try again later."
 msgstr ""
 
@@ -27330,6 +27336,9 @@ msgstr ""
 msgid "Private projects can be created in your personal namespace with:"
 msgstr ""
 
+msgid "Problem with %{name} command: %{message}."
+msgstr ""
+
 msgid "Proceed"
 msgstr ""
 

spec/services/quick_actions/interpret_service_spec.rb

@@ -485,6 +485,8 @@ RSpec.describe QuickActions::InterpretService do
     end
 
     shared_examples 'failed command' do |error_msg|
+      let(:match_msg) { error_msg ? eq(error_msg) : be_empty }
+
       it 'populates {} if content contains an unsupported command' do
         _, updates, _ = service.execute(content, issuable)
 
@@ -494,11 +496,7 @@ RSpec.describe QuickActions::InterpretService do
       it "returns #{error_msg || 'an empty'} message" do
         _, _, message = service.execute(content, issuable)
 
-        if error_msg
-          expect(message).to eq(error_msg)
-        else
-          expect(message).to be_empty
-        end
+        expect(message).to match_msg
       end
     end
 
@@ -887,9 +885,10 @@ RSpec.describe QuickActions::InterpretService do
       end
     end
 
-    it_behaves_like 'failed command', "Failed to assign a user because no user was found." do
+    it_behaves_like 'failed command', 'a parse error' do
       let(:content) { '/assign @abcd1234' }
       let(:issuable) { issue }
+      let(:match_msg) { eq "Could not apply assign command. Failed to find users for '@abcd1234'." }
     end
 
     it_behaves_like 'failed command', "Failed to assign a user because no user was found." do
@@ -953,7 +952,9 @@ RSpec.describe QuickActions::InterpretService do
       context 'with an incorrect user' do
         let(:content) { '/assign_reviewer @abcd1234' }
 
-        it_behaves_like 'failed command', "Failed to assign a reviewer because no user was found."
+        it_behaves_like 'failed command', 'a parse error' do
+          let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for '@abcd1234'." }
+        end
       end
 
       context 'with the "reviewer" alias' do
@@ -971,13 +972,16 @@ RSpec.describe QuickActions::InterpretService do
       context 'with no user' do
         let(:content) { '/assign_reviewer' }
 
-        it_behaves_like 'failed command', "Failed to assign a reviewer because no user was found."
+        it_behaves_like 'failed command', "Failed to assign a reviewer because no user was specified."
       end
 
-      context 'includes only the user reference with extra text' do
-        let(:content) { "/assign_reviewer @#{developer.username} do it!" }
+      context 'with extra text' do
+        let(:arg) { "@#{developer.username} do it!" }
+        let(:content) { "/assign_reviewer #{arg}" }
 
-        it_behaves_like 'assign_reviewer command'
+        it_behaves_like 'failed command', 'a parse error' do
+          let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for '#{arg}'." }
+        end
       end
     end
 
@@ -2317,12 +2321,41 @@ RSpec.describe QuickActions::InterpretService do
     end
 
     describe 'assign command' do
-      let(:content) { "/assign @#{developer.username} do it!" }
+      shared_examples 'assigns developer' do
+        it 'tells us we will assign the developer' do
+          _, explanations = service.explain(content, merge_request)
 
-      it 'includes only the user reference' do
-        _, explanations = service.explain(content, merge_request)
+          expect(explanations).to eq(["Assigns @#{developer.username}."])
+        end
+      end
+
+      context 'when using a reference' do
+        let(:content) { "/assign @#{developer.username}" }
+
+        include_examples 'assigns developer'
+      end
+
+      context 'when using a bare username' do
+        let(:content) { "/assign #{developer.username}" }
+
+        include_examples 'assigns developer'
+      end
 
-        expect(explanations).to eq(["Assigns @#{developer.username}."])
+      context 'when using me' do
+        let(:content) { "/assign me" }
+
+        include_examples 'assigns developer'
+      end
+
+      context 'when there are unparseable arguments' do
+        let(:arg) { "#{developer.username} to this issue" }
+        let(:content) { "/assign #{arg}" }
+
+        it 'tells us why we cannot do that' do
+          _, explanations = service.explain(content, merge_request)
+
+          expect(explanations).to eq ["Problem with assign command: Failed to find users for '#{arg}'."]
+        end
       end
     end