Refactor slash command definition

11 files changed, 190 insertions, 164 deletions

diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 9c387fd3daa..93338dba51e 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -134,8 +134,22 @@ class ProjectsController < Projects::ApplicationController
   end
 
   def autocomplete_sources
-    autocomplete = ::Projects::AutocompleteService.new(@project, current_user, params)
-    participants = ::Projects::ParticipantsService.new(@project, current_user, params).execute
+    noteable =
+      case params[:type]
+      when 'Issue'
+        IssuesFinder.new(current_user, project_id: project.id, state: 'all').
+          execute.find_by(iid: params[:type_id])
+      when 'MergeRequest'
+        MergeRequestsFinder.new(current_user, project_id: project.id, state: 'all').
+          execute.find_by(iid: params[:type_id])
+      when 'Commit'
+        project.commit(params[:type_id])
+      else
+        nil
+      end
+
+    autocomplete = ::Projects::AutocompleteService.new(@project, current_user)
+    participants = ::Projects::ParticipantsService.new(@project, current_user).execute(noteable)
 
     @suggestions = {
       emojis: Gitlab::AwardEmoji.urls,
@@ -144,7 +158,7 @@ class ProjectsController < Projects::ApplicationController
       mergerequests: autocomplete.merge_requests,
       labels: autocomplete.labels,
       members: participants,
-      commands: autocomplete.commands
+      commands: autocomplete.commands(noteable, params[:type])
     }
 
     respond_to do |format|
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 1a01b333366..aa08eef081c 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -94,10 +94,13 @@ class IssuableBaseService < BaseService
   end
 
   def merge_slash_commands_into_params!(issuable)
-    commands = SlashCommands::InterpretService.new(project, current_user).
+    description, command_params =
+      SlashCommands::InterpretService.new(project, current_user).
       execute(params[:description], issuable)
 
-    params.merge!(commands)
+    params[:description] = description
+
+    params.merge!(command_params)
   end
 
   def create_issuable(issuable, attributes)
diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index 1b2d63034b8..f7cf4a8edc0 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -15,7 +15,9 @@ module Notes
       # **before** we save the note because if the note consists of commands
       # only, there is no need be create a note!
       slash_commands_service = SlashCommandsService.new(project, current_user)
-      commands = slash_commands_service.extract_commands(note)
+      content, command_params = slash_commands_service.extract_commands(note)
+
+      note.note = content
 
       if note.save
         # Finish the harder work in the background
@@ -25,7 +27,7 @@ module Notes
 
       # We must add the error after we call #save because errors are reset
       # when #save is called
-      if slash_commands_service.execute(commands, note) && note.note.blank?
+      if slash_commands_service.execute(command_params, note) && note.note.blank?
         note.errors.add(:commands_only, 'Your commands are being executed.')
       end
 
diff --git a/app/services/notes/slash_commands_service.rb b/app/services/notes/slash_commands_service.rb
index ebced9577d8..f2c43775b72 100644
--- a/app/services/notes/slash_commands_service.rb
+++ b/app/services/notes/slash_commands_service.rb
@@ -1,6 +1,5 @@
 module Notes
   class SlashCommandsService < BaseService
-
     UPDATE_SERVICES = {
       'Issue' => Issues::UpdateService,
       'MergeRequest' => MergeRequests::UpdateService
@@ -15,11 +14,11 @@ module Notes
         execute(note.note, note.noteable)
     end
 
-    def execute(commands, note)
-      if commands.any?
-        @noteable_update_service.new(project, current_user, commands).
-          execute(note.noteable)
-      end
+    def execute(command_params, note)
+      return if command_params.empty?
+
+      @noteable_update_service.new(project, current_user, command_params).
+        execute(note.noteable)
     end
   end
 end
diff --git a/app/services/projects/autocomplete_service.rb b/app/services/projects/autocomplete_service.rb
index 477c999eff4..cb85ee6694d 100644
--- a/app/services/projects/autocomplete_service.rb
+++ b/app/services/projects/autocomplete_service.rb
@@ -1,7 +1,7 @@
 module Projects
   class AutocompleteService < BaseService
     def issues
-      @project.issues.visible_to_user(current_user).opened.select([:iid, :title])
+      IssuesFinder.new(current_user, project_id: project.id, state: 'opened').execute.select([:iid, :title])
     end
 
     def milestones
@@ -9,42 +9,34 @@ module Projects
     end
 
     def merge_requests
-      @project.merge_requests.opened.select([:iid, :title])
+      MergeRequestsFinder.new(current_user, project_id: project.id, state: 'opened').execute.select([:iid, :title])
     end
 
     def labels
       @project.labels.select([:title, :color])
     end
 
-    def commands
-      # We don't return commands when editing an issue or merge request
-      # This should be improved by not enabling autocomplete at the JS-level
-      # following this suggestion: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5021#note_13837384
-      return [] if !target || %w[edit update].include?(params[:action_name])
+    def commands(noteable, type)
+      noteable ||=
+        case type
+        when 'Issue'
+          @project.issues.build
+        when 'MergeRequest'
+          @project.merge_requests.build
+        end
 
-      SlashCommands::InterpretService.command_definitions(
+      return [] unless noteable && noteable.is_a?(Issuable)
+
+      opts = {
         project: project,
-        noteable: target,
+        noteable: noteable,
         current_user: current_user
-      )
-    end
+      }
+      SlashCommands::InterpretService.command_definitions.map do |definition|
+        next unless definition.available?(opts)
 
-    private
-
-    def target
-      @target ||= begin
-        noteable_id = params[:type_id]
-        case params[:type]
-        when 'Issue'
-          IssuesFinder.new(current_user, project_id: project.id, state: 'all').
-            execute.find_or_initialize_by(iid: noteable_id)
-        when 'MergeRequest'
-          MergeRequestsFinder.new(current_user, project_id: project.id, state: 'all').
-            execute.find_or_initialize_by(iid: noteable_id)
-        else
-          nil
-        end
-      end
+        definition.to_h(opts)
+      end.compact
     end
   end
 end
diff --git a/app/services/projects/participants_service.rb b/app/services/projects/participants_service.rb
index 1c8f2913e8b..d38328403c1 100644
--- a/app/services/projects/participants_service.rb
+++ b/app/services/projects/participants_service.rb
@@ -1,45 +1,28 @@
 module Projects
   class ParticipantsService < BaseService
-    attr_reader :noteable_type, :noteable_id
-
-    def execute
-      @noteable_type = params[:type]
-      @noteable_id = params[:type_id]
+    attr_reader :noteable
+    
+    def execute(noteable)
+      @noteable = noteable
 
       project_members = sorted(project.team.members)
-      participants = target_owner + participants_in_target + all_members + groups + project_members
+      participants = noteable_owner + participants_in_noteable + all_members + groups + project_members
       participants.uniq
     end
 
-    def target
-      @target ||=
-        case noteable_type
-        when 'Issue'
-          IssuesFinder.new(current_user, project_id: project.id, state: 'all').
-            execute.find_by(iid: noteable_id)
-        when 'MergeRequest'
-          MergeRequestsFinder.new(current_user, project_id: project.id, state: 'all').
-            execute.find_by(iid: noteable_id)
-        when 'Commit'
-          project.commit(noteable_id)
-        else
-          nil
-        end
-    end
-
-    def target_owner
-      return [] unless target && target.author.present?
+    def noteable_owner
+      return [] unless noteable && noteable.author.present?
 
       [{
-        name: target.author.name,
-        username: target.author.username
+        name: noteable.author.name,
+        username: noteable.author.username
       }]
     end
 
-    def participants_in_target
-      return [] unless target
+    def participants_in_noteable
+      return [] unless noteable
 
-      users = target.participants(current_user)
+      users = noteable.participants(current_user)
       sorted(users)
     end
 
diff --git a/app/services/slash_commands/interpret_service.rb b/app/services/slash_commands/interpret_service.rb
index 112bebe423a..a2b92d70f9f 100644
--- a/app/services/slash_commands/interpret_service.rb
+++ b/app/services/slash_commands/interpret_service.rb
@@ -10,20 +10,28 @@ module SlashCommands
       @noteable = noteable
       @updates = {}
 
-      commands = extractor(noteable: noteable).extract_commands!(content)
-      commands.each do |command, *args|
-        execute_command(command, *args)
+      opts = {
+        noteable:     noteable,
+        current_user: current_user,
+        project:      project
+      }
+
+      content, commands = extractor.extract_commands(content, opts)
+
+      commands.each do |name, *args|
+        definition = self.class.command_definitions_by_name[name.to_sym]
+        next unless definition
+
+        definition.execute(self, opts, *args)
       end
 
-      @updates
+      [content, @updates]
     end
 
     private
 
-    def extractor(opts = {})
-      opts.merge!(current_user: current_user, project: project)
-
-      Gitlab::SlashCommands::Extractor.new(self.class.command_names(opts))
+    def extractor
+      Gitlab::SlashCommands::Extractor.new(self.class.command_definitions)
     end
 
     desc do
diff --git a/lib/gitlab/slash_commands/command_definition.rb b/lib/gitlab/slash_commands/command_definition.rb
new file mode 100644
index 00000000000..5dec6c91869
--- /dev/null
+++ b/lib/gitlab/slash_commands/command_definition.rb
@@ -0,0 +1,57 @@
+module Gitlab
+  module SlashCommands
+    class CommandDefinition
+      attr_accessor :name, :aliases, :description, :params, :condition_block, :action_block
+
+      def valid?
+        name.present?
+      end
+
+      def all_names
+        [name, *aliases]
+      end
+
+      def noop?
+        action_block.nil?
+      end
+
+      def available?(opts)
+        return true unless condition_block
+
+        context = OpenStruct.new(opts)
+        context.instance_exec(&condition_block)
+      end
+
+      def to_description(opts)
+        return description unless description.respond_to?(:call)
+
+        context = OpenStruct.new(opts)
+        context.instance_exec(&description) rescue ''
+      end
+
+      def execute(context, opts, *args)
+        return if noop? || !available?(opts)
+
+        block_arity = action_block.arity
+        return unless block_arity == -1 || block_arity == args.size
+
+        context.instance_exec(*args, &action_block)
+      end
+
+      def to_h(opts)
+        desc = description
+        if desc.respond_to?(:call)
+          context = OpenStruct.new(opts)
+          desc = context.instance_exec(&desc) rescue ''
+        end
+
+        {
+          name: name,
+          aliases: aliases,
+          description: desc,
+          params: params
+        }
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/slash_commands/dsl.rb b/lib/gitlab/slash_commands/dsl.rb
index ce659aff1da..58ba7027f84 100644
--- a/lib/gitlab/slash_commands/dsl.rb
+++ b/lib/gitlab/slash_commands/dsl.rb
@@ -4,64 +4,16 @@ module Gitlab
       extend ActiveSupport::Concern
 
       included do
-        cattr_accessor :definitions
-      end
-
-      def execute_command(name, *args)
-        name = name.to_sym
-        cmd_def = self.class.definitions.find do |cmd_def|
-          self.class.command_name_and_aliases(cmd_def).include?(name)
+        cattr_accessor :command_definitions, instance_accessor: false do
+          []
         end
-        return unless cmd_def && cmd_def[:action_block]
-        return if self.class.command_unavailable?(cmd_def, self)
 
-        block_arity = cmd_def[:action_block].arity
-        if block_arity == -1 || block_arity == args.size
-          instance_exec(*args, &cmd_def[:action_block])
+        cattr_accessor :command_definitions_by_name, instance_accessor: false do
+          {}
         end
       end
 
       class_methods do
-        # This method is used to generate the autocompletion menu.
-        # It returns no-op slash commands (such as `/cc`).
-        def command_definitions(opts = {})
-          self.definitions.map do |cmd_def|
-            context = OpenStruct.new(opts)
-            next if command_unavailable?(cmd_def, context)
-
-            cmd_def = cmd_def.dup
-
-            if cmd_def[:description].respond_to?(:call)
-              cmd_def[:description] = context.instance_exec(&cmd_def[:description]) rescue ''
-            end
-
-            cmd_def
-          end.compact
-        end
-
-        # This method is used to generate a list of valid commands in the current
-        # context of `opts`.
-        # It excludes no-op slash commands (such as `/cc`).
-        # This list can then be given to `Gitlab::SlashCommands::Extractor`.
-        def command_names(opts = {})
-          self.definitions.flat_map do |cmd_def|
-            next if cmd_def[:opts].fetch(:noop, false)
-
-            context = OpenStruct.new(opts)
-            next if command_unavailable?(cmd_def, context)
-
-            command_name_and_aliases(cmd_def)
-          end.compact
-        end
-
-        def command_unavailable?(cmd_def, context)
-          cmd_def[:condition_block] && !context.instance_exec(&cmd_def[:condition_block])
-        end
-
-        def command_name_and_aliases(cmd_def)
-          [cmd_def[:name], *cmd_def[:aliases]]
-        end
-
         # Allows to give a description to the next slash command.
         # This description is shown in the autocomplete menu.
         # It accepts a block that will be evaluated with the context given to
@@ -119,19 +71,23 @@ module Gitlab
         #     # Awesome code block
         #   end
         def command(*command_names, &block)
-          opts = command_names.extract_options!
           name, *aliases = command_names
 
-          self.definitions ||= []
-          self.definitions << {
-            name: name,
-            aliases: aliases,
-            description: @description || '',
-            params: @params || [],
-            condition_block: @condition_block,
-            action_block: block,
-            opts: opts
-          }
+          definition = CommandDefinition.new
+          definition.name            =  name
+          definition.aliases         =  aliases
+          definition.description     =  @description || ''
+          definition.params          =  @params || []
+          definition.condition_block =  @condition_block
+          definition.action_block    =  block
+
+          return unless definition.valid?
+
+          self.command_definitions << definition
+
+          definition.all_names.each do |name|
+            self.command_definitions_by_name[name] = definition
+          end
 
           @description = nil
           @params = nil
diff --git a/lib/gitlab/slash_commands/extractor.rb b/lib/gitlab/slash_commands/extractor.rb
index ce0a2eba535..a6838cb5e7c 100644
--- a/lib/gitlab/slash_commands/extractor.rb
+++ b/lib/gitlab/slash_commands/extractor.rb
@@ -7,10 +7,10 @@ module Gitlab
     # extractor = Gitlab::SlashCommands::Extractor.new([:open, :assign, :labels])
     # ```
     class Extractor
-      attr_reader :command_names
+      attr_reader :command_definitions
 
-      def initialize(command_names)
-        @command_names = command_names
+      def initialize(command_definitions)
+        @command_definitions = command_definitions
       end
 
       # Extracts commands from content and return an array of commands.
@@ -26,16 +26,18 @@ module Gitlab
       # ```
       # extractor = Gitlab::SlashCommands::Extractor.new([:open, :assign, :labels])
       # msg = %(hello\n/labels ~foo ~"bar baz"\nworld)
-      # commands = extractor.extract_commands! #=> [['labels', '~foo ~"bar baz"']]
+      # commands = extractor.extract_commands(msg) #=> [['labels', '~foo ~"bar baz"']]
       # msg #=> "hello\nworld"
       # ```
-      def extract_commands!(content)
+      def extract_commands(content, opts)
         return [] unless content
 
+        content = content.dup
+
         commands = []
 
         content.delete!("\r")
-        content.gsub!(commands_regex) do
+        content.gsub!(commands_regex(opts)) do
           if $~[:cmd]
             commands << [$~[:cmd], $~[:args]].reject(&:blank?)
             ''
@@ -44,11 +46,19 @@ module Gitlab
           end
         end
 
-        commands
+        [content.strip, commands]
       end
 
       private
 
+      def command_names(opts)
+        command_definitions.flat_map do |command|
+          next if command.noop?
+
+          command.all_names
+        end.compact
+      end
+
       # Builds a regular expression to match known commands.
       # First match group captures the command name and
       # second match group captures its arguments.
@@ -56,7 +66,9 @@ module Gitlab
       # It looks something like:
       #
       #   /^\/(?<cmd>close|reopen|...)(?:( |$))(?<args>[^\/\n]*)(?:\n|$)/
-      def commands_regex
+      def commands_regex(opts)
+        names = command_names(opts).map(&:to_s)
+
         @commands_regex ||= %r{
             (?<code>
               # Code blocks:
@@ -95,7 +107,7 @@ module Gitlab
               # Command not in a blockquote, blockcode, or HTML tag:
               # /close
 
-              ^\/(?<cmd>#{command_names.join('|')})(?:(\ |$))(?<args>[^\/\n]*)(?:\n|$)
+              ^\/(?<cmd>#{Regexp.union(names)})(?:$|\ (?<args>[^\/\n]*)$)
             )
         }mx
       end
diff --git a/spec/lib/gitlab/slash_commands/extractor_spec.rb b/spec/lib/gitlab/slash_commands/extractor_spec.rb
index ac7296bdba1..8a6801205fa 100644
--- a/spec/lib/gitlab/slash_commands/extractor_spec.rb
+++ b/spec/lib/gitlab/slash_commands/extractor_spec.rb
@@ -5,7 +5,7 @@ describe Gitlab::SlashCommands::Extractor do
 
   shared_examples 'command with no argument' do
     it 'extracts command' do
-      commands = extractor.extract_commands!(original_msg)
+      commands = extractor.extract_commands(original_msg)
 
       expect(commands).to eq [['open']]
       expect(original_msg).to eq final_msg
@@ -14,7 +14,7 @@ describe Gitlab::SlashCommands::Extractor do
 
   shared_examples 'command with a single argument' do
     it 'extracts command' do
-      commands = extractor.extract_commands!(original_msg)
+      commands = extractor.extract_commands(original_msg)
 
       expect(commands).to eq [['assign', '@joe']]
       expect(original_msg).to eq final_msg
@@ -23,14 +23,14 @@ describe Gitlab::SlashCommands::Extractor do
 
   shared_examples 'command with multiple arguments' do
     it 'extracts command' do
-      commands = extractor.extract_commands!(original_msg)
+      commands = extractor.extract_commands(original_msg)
 
       expect(commands).to eq [['labels', '~foo ~"bar baz" label']]
       expect(original_msg).to eq final_msg
     end
   end
 
-  describe '#extract_commands!' do
+  describe '#extract_commands' do
     describe 'command with no argument' do
       context 'at the start of content' do
         it_behaves_like 'command with no argument' do
@@ -49,7 +49,7 @@ describe Gitlab::SlashCommands::Extractor do
       context 'in the middle of a line' do
         it 'does not extract command' do
           msg = "hello\nworld /open"
-          commands = extractor.extract_commands!(msg)
+          commands = extractor.extract_commands(msg)
 
           expect(commands).to be_empty
           expect(msg).to eq "hello\nworld /open"
@@ -82,7 +82,7 @@ describe Gitlab::SlashCommands::Extractor do
       context 'in the middle of a line' do
         it 'does not extract command' do
           msg = "hello\nworld /assign @joe"
-          commands = extractor.extract_commands!(msg)
+          commands = extractor.extract_commands(msg)
 
           expect(commands).to be_empty
           expect(msg).to eq "hello\nworld /assign @joe"
@@ -99,7 +99,7 @@ describe Gitlab::SlashCommands::Extractor do
       context 'when argument is not separated with a space' do
         it 'does not extract command' do
           msg = "hello\n/assign@joe\nworld"
-          commands = extractor.extract_commands!(msg)
+          commands = extractor.extract_commands(msg)
 
           expect(commands).to be_empty
           expect(msg).to eq "hello\n/assign@joe\nworld"
@@ -125,7 +125,7 @@ describe Gitlab::SlashCommands::Extractor do
       context 'in the middle of a line' do
         it 'does not extract command' do
           msg = %(hello\nworld /labels ~foo ~"bar baz" label)
-          commands = extractor.extract_commands!(msg)
+          commands = extractor.extract_commands(msg)
 
           expect(commands).to be_empty
           expect(msg).to eq %(hello\nworld /labels ~foo ~"bar baz" label)
@@ -142,7 +142,7 @@ describe Gitlab::SlashCommands::Extractor do
       context 'when argument is not separated with a space' do
         it 'does not extract command' do
           msg = %(hello\n/labels~foo ~"bar baz" label\nworld)
-          commands = extractor.extract_commands!(msg)
+          commands = extractor.extract_commands(msg)
 
           expect(commands).to be_empty
           expect(msg).to eq %(hello\n/labels~foo ~"bar baz" label\nworld)
@@ -152,7 +152,7 @@ describe Gitlab::SlashCommands::Extractor do
 
     it 'extracts command with multiple arguments and various prefixes' do
       msg = %(hello\n/power @user.name %9.10 ~"bar baz.2"\nworld)
-      commands = extractor.extract_commands!(msg)
+      commands = extractor.extract_commands(msg)
 
       expect(commands).to eq [['power', '@user.name %9.10 ~"bar baz.2"']]
       expect(msg).to eq "hello\nworld"
@@ -160,7 +160,7 @@ describe Gitlab::SlashCommands::Extractor do
 
     it 'extracts multiple commands' do
       msg = %(hello\n/power @user.name %9.10 ~"bar baz.2" label\nworld\n/open)
-      commands = extractor.extract_commands!(msg)
+      commands = extractor.extract_commands(msg)
 
       expect(commands).to eq [['power', '@user.name %9.10 ~"bar baz.2" label'], ['open']]
       expect(msg).to eq "hello\nworld\n"
@@ -168,7 +168,7 @@ describe Gitlab::SlashCommands::Extractor do
 
     it 'does not alter original content if no command is found' do
       msg = 'Fixes #123'
-      commands = extractor.extract_commands!(msg)
+      commands = extractor.extract_commands(msg)
 
       expect(commands).to be_empty
       expect(msg).to eq 'Fixes #123'
@@ -177,7 +177,7 @@ describe Gitlab::SlashCommands::Extractor do
     it 'does not extract commands inside a blockcode' do
       msg = "Hello\r\n```\r\nThis is some text\r\n/close\r\n/assign @user\r\n```\r\n\r\nWorld"
       expected = msg.delete("\r")
-      commands = extractor.extract_commands!(msg)
+      commands = extractor.extract_commands(msg)
 
       expect(commands).to be_empty
       expect(msg).to eq expected
@@ -186,7 +186,7 @@ describe Gitlab::SlashCommands::Extractor do
     it 'does not extract commands inside a blockquote' do
       msg = "Hello\r\n>>>\r\nThis is some text\r\n/close\r\n/assign @user\r\n>>>\r\n\r\nWorld"
       expected = msg.delete("\r")
-      commands = extractor.extract_commands!(msg)
+      commands = extractor.extract_commands(msg)
 
       expect(commands).to be_empty
       expect(msg).to eq expected
@@ -195,7 +195,7 @@ describe Gitlab::SlashCommands::Extractor do
     it 'does not extract commands inside a HTML tag' do
       msg = "Hello\r\n<div>\r\nThis is some text\r\n/close\r\n/assign @user\r\n</div>\r\n\r\nWorld"
       expected = msg.delete("\r")
-      commands = extractor.extract_commands!(msg)
+      commands = extractor.extract_commands(msg)
 
       expect(commands).to be_empty
       expect(msg).to eq expected