Introduce concept of parse errors for quick actions

This allows quick actions to signal that they cannot be executed due to
failure to interpret the arguments passed to them.

An initial use is to guard against destructive updates caused by failure
to parse/find users in assignment commands.

As a result, behavior with invalid arguments changes, from tolerance
of bad arguments to stricter validation. Some operations that previously
would succeed (e.g. `/assign me to this issue`) will now fail.

Changelog: fixed

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

@@ -9948,6 +9948,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 ""
 
@@ -14778,7 +14781,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."
@@ -14844,6 +14847,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 ""
 
@@ -27316,6 +27322,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