From c85a19f920da1b544bbfae344145503c25e71048 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 7 Nov 2018 13:33:42 +0100 Subject: [PATCH] Allow limiting quick actions to execute Sometimes we don't want to trigger any quick actions that cause side effects. For example when building a record to validate. This allows listing the quick actions that need to be performed. --- app/services/issuable_base_service.rb | 4 ++-- app/services/merge_requests/build_service.rb | 4 +++- .../quick_actions/interpret_service.rb | 4 ++-- lib/gitlab/quick_actions/extractor.rb | 14 +++++++++----- .../gitlab/quick_actions/extractor_spec.rb | 19 +++++++++++++++++++ .../quick_actions/interpret_service_spec.rb | 9 +++++++++ 6 files changed, 44 insertions(+), 10 deletions(-) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index c388913ae65..e32e262ac31 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -126,12 +126,12 @@ class IssuableBaseService < BaseService merge_quick_actions_into_params!(issuable) end - def merge_quick_actions_into_params!(issuable) + def merge_quick_actions_into_params!(issuable, only: nil) original_description = params.fetch(:description, issuable.description) description, command_params = QuickActions::InterpretService.new(project, current_user) - .execute(original_description, issuable) + .execute(original_description, issuable, only: only) # Avoid a description already set on an issuable to be overwritten by a nil params[:description] = description if description diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 6c9e566109a..6c69452e2ab 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -7,7 +7,9 @@ module MergeRequests def execute @params_issue_iid = params.delete(:issue_iid) self.merge_request = MergeRequest.new - merge_quick_actions_into_params!(merge_request) + # TODO: this should handle all quick actions that don't have side effects + # https://gitlab.com/gitlab-org/gitlab-ce/issues/53658 + merge_quick_actions_into_params!(merge_request, only: [:target_branch]) merge_request.assign_attributes(params) merge_request.author = current_user diff --git a/app/services/quick_actions/interpret_service.rb b/app/services/quick_actions/interpret_service.rb index eb431c36807..9c81de7e90e 100644 --- a/app/services/quick_actions/interpret_service.rb +++ b/app/services/quick_actions/interpret_service.rb @@ -23,13 +23,13 @@ module QuickActions # Takes a text and interprets the commands that are extracted from it. # Returns the content without commands, and hash of changes to be applied to a record. - def execute(content, issuable) + def execute(content, issuable, only: nil) return [content, {}] unless current_user.can?(:use_quick_actions) @issuable = issuable @updates = {} - content, commands = extractor.extract_commands(content) + content, commands = extractor.extract_commands(content, only: only) extract_updates(commands) [content, @updates] diff --git a/lib/gitlab/quick_actions/extractor.rb b/lib/gitlab/quick_actions/extractor.rb index 30c6806b68e..59f8dd889aa 100644 --- a/lib/gitlab/quick_actions/extractor.rb +++ b/lib/gitlab/quick_actions/extractor.rb @@ -29,7 +29,7 @@ module Gitlab # commands = extractor.extract_commands(msg) #=> [['labels', '~foo ~"bar baz"']] # msg #=> "hello\nworld" # ``` - def extract_commands(content) + def extract_commands(content, only: nil) return [content, []] unless content content = content.dup @@ -37,7 +37,7 @@ module Gitlab commands = [] content.delete!("\r") - content.gsub!(commands_regex) do + content.gsub!(commands_regex(only: only)) do if $~[:cmd] commands << [$~[:cmd].downcase, $~[:arg]].reject(&:blank?) '' @@ -60,8 +60,8 @@ module Gitlab # It looks something like: # # /^\/(?close|reopen|...)(?:( |$))(?[^\/\n]*)(?:\n|$)/ - def commands_regex - names = command_names.map(&:to_s) + def commands_regex(only:) + names = command_names(limit_to_commands: only).map(&:to_s) @commands_regex ||= %r{ (? @@ -133,10 +133,14 @@ module Gitlab [content, commands] end - def command_names + def command_names(limit_to_commands:) command_definitions.flat_map do |command| next if command.noop? + if limit_to_commands && (command.all_names & limit_to_commands).empty? + next + end + command.all_names end.compact end diff --git a/spec/lib/gitlab/quick_actions/extractor_spec.rb b/spec/lib/gitlab/quick_actions/extractor_spec.rb index 0166f6c2ee0..873bb359d6e 100644 --- a/spec/lib/gitlab/quick_actions/extractor_spec.rb +++ b/spec/lib/gitlab/quick_actions/extractor_spec.rb @@ -272,5 +272,24 @@ describe Gitlab::QuickActions::Extractor do expect(commands).to be_empty expect(msg).to eq expected end + + it 'limits to passed commands when they are passed' do + msg = <<~MSG.strip + Hello, we should only extract the commands passed + /reopen + /labels hello world + /power + MSG + expected_msg = <<~EXPECTED.strip + Hello, we should only extract the commands passed + /power + EXPECTED + expected_commands = [['reopen'], ['labels', 'hello world']] + + msg, commands = extractor.extract_commands(msg, only: [:open, :labels]) + + expect(commands).to eq(expected_commands) + expect(msg).to eq expected_msg + end end end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index e513ee7ae44..5a7cafcb60f 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -1213,6 +1213,15 @@ describe QuickActions::InterpretService do end end end + + it 'limits to commands passed ' do + content = "/shrug\n/close" + + text, commands = service.execute(content, issue, only: [:shrug]) + + expect(commands).to be_empty + expect(text).to eq("#{described_class::SHRUG}\n/close") + end end describe '#explain' do