Accept blocks for `.desc` and `.condition` slash commands DSL

Also, pass options as instance variables, making the DSL more
user-friendly / natural.
Signed-off-by: Rémy Coutable <remy@rymai.me>

app/services/slash_commands/interpret_service.rb

@@ -26,25 +26,23 @@ module SlashCommands
       Gitlab::SlashCommands::Extractor.new(self.class.command_names(opts))
     end
 
-    desc ->(opts) { "Close this #{opts[:noteable].to_ability_name.humanize(capitalize: false)}" }
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:noteable].open? &&
-      opts[:current_user] &&
-      opts[:project] &&
-      opts[:current_user].can?(:"update_#{opts[:noteable].to_ability_name}", opts[:project])
+    desc do
+      "Close this #{noteable.to_ability_name.humanize(capitalize: false)}"
+    end
+    condition do
+      noteable.open? &&
+      current_user.can?(:"update_#{noteable.to_ability_name}", project)
     end
     command :close do
       @updates[:state_event] = 'close'
     end
 
-    desc ->(opts) { "Reopen this #{opts[:noteable].to_ability_name.humanize(capitalize: false)}" }
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:noteable].closed? &&
-      opts[:current_user] &&
-      opts[:project] &&
-      opts[:current_user].can?(:"update_#{opts[:noteable].to_ability_name}", opts[:project])
+    desc do
+      "Reopen this #{noteable.to_ability_name.humanize(capitalize: false)}"
+    end
+    condition do
+      noteable.closed? &&
+      current_user.can?(:"update_#{noteable.to_ability_name}", project)
     end
     command :open, :reopen do
       @updates[:state_event] = 'reopen'
@@ -52,12 +50,9 @@ module SlashCommands
 
     desc 'Change title'
     params '<New title>'
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:noteable].persisted? &&
-      opts[:current_user] &&
-      opts[:project] &&
-      opts[:current_user].can?(:"update_#{opts[:noteable].to_ability_name}", opts[:project])
+    condition do
+      noteable.persisted? &&
+      current_user.can?(:"update_#{noteable.to_ability_name}", project)
     end
     command :title do |title_param|
       @updates[:title] = title_param
@@ -65,11 +60,8 @@ module SlashCommands
 
     desc 'Assign'
     params '@user'
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:current_user] &&
-      opts[:project] &&
-      opts[:current_user].can?(:"admin_#{opts[:noteable].to_ability_name}", opts[:project])
+    condition do
+      current_user.can?(:"admin_#{noteable.to_ability_name}", project)
     end
     command :assign, :reassign do |assignee_param|
       user = extract_references(assignee_param, :user).first
@@ -79,12 +71,9 @@ module SlashCommands
     end
 
     desc 'Remove assignee'
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:noteable].assignee_id? &&
-      opts[:current_user] &&
-      opts[:project] &&
-      opts[:current_user].can?(:"admin_#{opts[:noteable].to_ability_name}", opts[:project])
+    condition do
+      noteable.assignee_id? &&
+      current_user.can?(:"admin_#{noteable.to_ability_name}", project)
     end
     command :unassign, :remove_assignee do
       @updates[:assignee_id] = nil
@@ -92,12 +81,9 @@ module SlashCommands
 
     desc 'Set milestone'
     params '%"milestone"'
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:current_user] &&
-      opts[:project] &&
-      opts[:current_user].can?(:"admin_#{opts[:noteable].to_ability_name}", opts[:project]) &&
-      opts[:project].milestones.active.any?
+    condition do
+      current_user.can?(:"admin_#{noteable.to_ability_name}", project) &&
+      project.milestones.active.any?
     end
     command :milestone do |milestone_param|
       milestone = extract_references(milestone_param, :milestone).first
@@ -107,12 +93,9 @@ module SlashCommands
     end
 
     desc 'Remove milestone'
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:noteable].milestone_id? &&
-      opts[:current_user] &&
-      opts[:project] &&
-      opts[:current_user].can?(:"admin_#{opts[:noteable].to_ability_name}", opts[:project])
+    condition do
+      noteable.milestone_id? &&
+      current_user.can?(:"admin_#{noteable.to_ability_name}", project)
     end
     command :clear_milestone, :remove_milestone do
       @updates[:milestone_id] = nil
@@ -120,12 +103,9 @@ module SlashCommands
 
     desc 'Add label(s)'
     params '~label1 ~"label 2"'
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:current_user] &&
-      opts[:project] &&
-      opts[:current_user].can?(:"admin_#{opts[:noteable].to_ability_name}", opts[:project]) &&
-      opts[:project].labels.any?
+    condition do
+      current_user.can?(:"admin_#{noteable.to_ability_name}", project) &&
+      project.labels.any?
     end
     command :label, :labels do |labels_param|
       label_ids = find_label_ids(labels_param)
@@ -136,12 +116,9 @@ module SlashCommands
 
     desc 'Remove label(s)'
     params '~label1 ~"label 2"'
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:noteable].labels.any? &&
-      opts[:current_user] &&
-      opts[:project] &&
-      opts[:current_user].can?(:"admin_#{opts[:noteable].to_ability_name}", opts[:project])
+    condition do
+      noteable.labels.any? &&
+      current_user.can?(:"admin_#{noteable.to_ability_name}", project)
     end
     command :unlabel, :remove_label, :remove_labels do |labels_param|
       label_ids = find_label_ids(labels_param)
@@ -151,55 +128,46 @@ module SlashCommands
     end
 
     desc 'Remove all labels'
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:noteable].labels.any? &&
-      opts[:current_user] &&
-      opts[:project] &&
-      opts[:current_user].can?(:"admin_#{opts[:noteable].to_ability_name}", opts[:project])
+    condition do
+      noteable.labels.any? &&
+      current_user.can?(:"admin_#{noteable.to_ability_name}", project)
     end
     command :clear_labels, :clear_label do
       @updates[:label_ids] = []
     end
 
     desc 'Add a todo'
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:noteable].persisted? &&
-      opts[:current_user] &&
-      !TodosFinder.new(opts[:current_user]).execute.exists?(target: opts[:noteable])
+    condition do
+      noteable.persisted? &&
+      current_user &&
+      !TodosFinder.new(current_user).execute.exists?(target: noteable)
     end
     command :todo do
       @updates[:todo_event] = 'add'
     end
 
     desc 'Mark todo as done'
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:current_user] &&
-      TodosFinder.new(opts[:current_user]).execute.exists?(target: opts[:noteable])
+    condition do
+      current_user &&
+      TodosFinder.new(current_user).execute.exists?(target: noteable)
     end
     command :done do
       @updates[:todo_event] = 'done'
     end
 
     desc 'Subscribe'
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:current_user] &&
-      opts[:noteable].persisted? &&
-      !opts[:noteable].subscribed?(opts[:current_user])
+    condition do
+      noteable.persisted? &&
+      !noteable.subscribed?(current_user)
     end
     command :subscribe do
       @updates[:subscription_event] = 'subscribe'
     end
 
     desc 'Unsubscribe'
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:current_user] &&
-      opts[:noteable].persisted? &&
-      opts[:noteable].subscribed?(opts[:current_user])
+    condition do
+      noteable.persisted? &&
+      noteable.subscribed?(current_user)
     end
     command :unsubscribe do
       @updates[:subscription_event] = 'unsubscribe'
@@ -207,12 +175,9 @@ module SlashCommands
 
     desc 'Set due date'
     params 'a date in natural language'
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:noteable].respond_to?(:due_date) &&
-      opts[:current_user] &&
-      opts[:project] &&
-      opts[:current_user].can?(:"update_#{opts[:noteable].to_ability_name}", opts[:project])
+    condition do
+      noteable.respond_to?(:due_date) &&
+      current_user.can?(:"update_#{noteable.to_ability_name}", project)
     end
     command :due_date, :due do |due_date_param|
       due_date = Chronic.parse(due_date_param).try(:to_date)
@@ -221,13 +186,10 @@ module SlashCommands
     end
 
     desc 'Remove due date'
-    condition ->(opts) do
-      opts[:noteable] &&
-      opts[:noteable].respond_to?(:due_date) &&
-      opts[:noteable].due_date? &&
-      opts[:current_user] &&
-      opts[:project] &&
-      opts[:current_user].can?(:"update_#{opts[:noteable].to_ability_name}", opts[:project])
+    condition do
+      noteable.respond_to?(:due_date) &&
+      noteable.due_date? &&
+      current_user.can?(:"update_#{noteable.to_ability_name}", project)
     end
     command :clear_due_date do
       @updates[:due_date] = nil
@@ -236,10 +198,7 @@ module SlashCommands
     # This is a dummy command, so that it appears in the autocomplete commands
     desc 'CC'
     params '@user'
-    noop true
-    command :cc do
-      return
-    end
+    command :cc, noop: true
 
     def find_label_ids(labels_param)
       extract_references(labels_param, :label).map(&:id)

lib/gitlab/slash_commands/dsl.rb

@@ -8,20 +8,27 @@ module Gitlab
       end
 
       module ClassMethods
+        # This method is used to generate the autocompletion menu
+        # It returns no-op slash commands (such as `/cc`)
         def command_definitions(opts = {})
           @command_definitions.map do |cmd_def|
-            next if cmd_def[:cond_lambda] && !cmd_def[:cond_lambda].call(opts)
+            context = OpenStruct.new(opts)
+            next if cmd_def[:cond_block] && !context.instance_exec(&cmd_def[:cond_block])
 
             cmd_def = cmd_def.dup
 
             if cmd_def[:description].present? && cmd_def[:description].respond_to?(:call)
-              cmd_def[:description] = cmd_def[:description].call(opts) rescue ''
+              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 = {})
           command_definitions(opts).flat_map do |command_definition|
             next if command_definition[:noop]
@@ -30,59 +37,88 @@ module Gitlab
           end.compact
         end
 
-        # Allows to give a description to the next slash command
-        def desc(text)
-          @description = text
+        # 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
+        # `.command_definitions` or `.command_names`.
+        #
+        # Example:
+        #
+        #   desc do
+        #     "This is a dynamic description for #{noteable.to_ability_name}"
+        #   end
+        #   command :command_key do |arguments|
+        #     # Awesome code block
+        #   end
+        def desc(text = '', &block)
+          @description = block_given? ? block : text
         end
 
-        # Allows to define params for the next slash command
+        # Allows to define params for the next slash command.
+        # These params are shown in the autocomplete menu.
+        #
+        # Example:
+        #
+        #   params "~label ~label2"
+        #   command :command_key do |arguments|
+        #     # Awesome code block
+        #   end
         def params(*params)
           @params = params
         end
 
-        # Allows to define if a command is a no-op, but should appear in autocomplete
-        def noop(noop)
-          @noop = noop
-        end
-
-        # Allows to define if a lambda to conditionally return an action
-        def condition(cond_lambda)
-          @cond_lambda = cond_lambda
-        end
-
-        # Registers a new command which is recognizeable
-        # from body of email or comment.
+        # Allows to define conditions that must be met in order for the command
+        # to be returned by `.command_names` & `.command_definitions`.
+        # It accepts a block that will be evaluated with the context given to
+        # `.command_definitions`, `.command_names`, and the actual command method.
+        #
         # Example:
         #
+        #   condition do
+        #     project.public?
+        #   end
         #   command :command_key do |arguments|
         #     # Awesome code block
         #   end
+        def condition(&block)
+          @cond_block = block
+        end
+
+        # Registers a new command which is recognizeable from body of email or
+        # comment.
+        # It accepts aliases and takes a block.
         #
+        # Example:
+        #
+        #   command :my_command, :alias_for_my_command do |arguments|
+        #     # Awesome code block
+        #   end
         def command(*command_names, &block)
+          opts = command_names.extract_options!
           command_name, *aliases = command_names
           proxy_method_name = "__#{command_name}__"
 
-          # This proxy method is needed because calling `return` from inside a
-          # block/proc, causes a `return` from the enclosing method or lambda,
-          # otherwise a LocalJumpError error is raised.
-          define_method(proxy_method_name, &block)
+          if block_given?
+            # This proxy method is needed because calling `return` from inside a
+            # block/proc, causes a `return` from the enclosing method or lambda,
+            # otherwise a LocalJumpError error is raised.
+            define_method(proxy_method_name, &block)
 
-          define_method(command_name) do |*args|
-            unless @cond_lambda.nil? || @cond_lambda.call(project: project, current_user: current_user, noteable: noteable)
-              return
-            end
+            define_method(command_name) do |*args|
+              return if @cond_block && !instance_exec(&@cond_block)
 
-            proxy_method = method(proxy_method_name)
+              proxy_method = method(proxy_method_name)
 
-            if proxy_method.arity == -1 || proxy_method.arity == args.size
-              instance_exec(*args, &proxy_method)
+              if proxy_method.arity == -1 || proxy_method.arity == args.size
+                instance_exec(*args, &proxy_method)
+              end
             end
-          end
 
-          private command_name
-          aliases.each do |alias_command|
-            alias_method alias_command, command_name
-            private alias_command
+            private command_name
+            aliases.each do |alias_command|
+              alias_method alias_command, command_name
+              private alias_command
+            end
           end
 
           command_definition = {
@@ -91,14 +127,13 @@ module Gitlab
             description: @description || '',
             params: @params || []
           }
-          command_definition[:noop] = @noop unless @noop.nil?
-          command_definition[:cond_lambda] = @cond_lambda unless @cond_lambda.nil?
+          command_definition[:noop] = opts[:noop] || false
+          command_definition[:cond_block] = @cond_block
           @command_definitions << command_definition
 
           @description = nil
           @params = nil
-          @noop = nil
-          @cond_lambda = nil
+          @cond_block = nil
         end
       end
     end

spec/lib/gitlab/slash_commands/dsl_spec.rb

 require 'spec_helper'
 
 describe Gitlab::SlashCommands::Dsl do
-  COND_LAMBDA = ->(opts) { opts[:project] == 'foo' }
   before :all do
-    DummyClass = Class.new do
+    DummyClass = Struct.new(:project) do
       include Gitlab::SlashCommands::Dsl
 
       desc 'A command with no args'
@@ -21,20 +20,21 @@ describe Gitlab::SlashCommands::Dsl do
         arg1
       end
 
-      desc ->(opts) { "A dynamic description for #{opts.fetch(:noteable)}" }
+      desc do
+        "A dynamic description for #{noteable.upcase}"
+      end
       params 'The first argument', 'The second argument'
       command :two_args do |arg1, arg2|
         [arg1, arg2]
       end
 
-      noop true
-      command :cc do |*args|
-        args
-      end
+      command :cc, noop: true
 
-      condition COND_LAMBDA
-      command :cond_action do |*args|
-        args
+      condition do
+        project == 'foo'
+      end
+      command :cond_action do |arg|
+        arg
       end
 
       command :wildcard do |*args|
@@ -42,17 +42,16 @@ describe Gitlab::SlashCommands::Dsl do
       end
     end
   end
-  let(:dummy) { DummyClass.new }
 
   describe '.command_definitions' do
     let(:base_expected) do
       [
-        { name: :no_args, aliases: [:none], description: 'A command with no args', params: [] },
-        { name: :returning, aliases: [], description: 'A command returning a value', params: [] },
-        { name: :one_arg, aliases: [:once, :first], description: '', params: ['The first argument'] },
-        { name: :two_args, aliases: [], description: '', params: ['The first argument', 'The second argument'] },
-        { name: :cc, aliases: [], description: '', params: [], noop: true },
-        { name: :wildcard, aliases: [], description: '', params: [] }
+        { name: :no_args, aliases: [:none], description: 'A command with no args', params: [], noop: false, cond_block: nil },
+        { name: :returning, aliases: [], description: 'A command returning a value', params: [], noop: false, cond_block: nil },
+        { name: :one_arg, aliases: [:once, :first], description: '', params: ['The first argument'], noop: false, cond_block: nil },
+        { name: :two_args, aliases: [], description: '', params: ['The first argument', 'The second argument'], noop: false, cond_block: nil },
+        { name: :cc, aliases: [], description: '', params: [], noop: true, cond_block: nil },
+        { name: :wildcard, aliases: [], description: '', params: [], noop: false, cond_block: nil}
       ]
     end
 
@@ -62,7 +61,7 @@ describe Gitlab::SlashCommands::Dsl do
 
     context 'with options passed' do
       context 'when condition is met' do
-        let(:expected) { base_expected << { name: :cond_action, aliases: [], description: '', params: [], cond_lambda: COND_LAMBDA } }
+        let(:expected) { base_expected << { name: :cond_action, aliases: [], description: '', params: [], noop: false, cond_block: a_kind_of(Proc) } }
 
         it 'returns an array with commands definitions' do
           expect(DummyClass.command_definitions(project: 'foo')).to match_array expected
@@ -77,7 +76,7 @@ describe Gitlab::SlashCommands::Dsl do
 
       context 'when description can be generated dynamically' do
         it 'returns an array with commands definitions with dynamic descriptions' do
-          base_expected[3][:description] = 'A dynamic description for merge request'
+          base_expected[3][:description] = 'A dynamic description for MERGE REQUEST'
 
           expect(DummyClass.command_definitions(noteable: 'merge request')).to match_array base_expected
         end
@@ -114,6 +113,8 @@ describe Gitlab::SlashCommands::Dsl do
     end
   end
 
+  let(:dummy) { DummyClass.new(nil) }
+
   describe 'command with no args' do
     context 'called with no args' do
       it 'succeeds' do
@@ -146,6 +147,28 @@ describe Gitlab::SlashCommands::Dsl do
     end
   end
 
+  describe 'noop command' do
+    it 'is not meant to be called directly' do
+      expect { dummy.__send__(:cc) }.to raise_error(NoMethodError)
+    end
+  end
+
+  describe 'command with condition' do
+    context 'when condition is not met' do
+      it 'returns nil' do
+        expect(dummy.__send__(:cond_action)).to be_nil
+      end
+    end
+
+    context 'when condition is met' do
+      let(:dummy) { DummyClass.new('foo') }
+
+      it 'succeeds' do
+        expect(dummy.__send__(:cond_action, 42)).to eq 42
+      end
+    end
+  end
+
   describe 'command with wildcard' do
     context 'called with no args' do
       it 'succeeds' do