Fixed specs and fixes based on failing specs

app/controllers/projects/issues_controller.rb

@@ -176,12 +176,7 @@ class Projects::IssuesController < Projects::ApplicationController
   protected
 
   def issue
-    @noteable = @issue ||=
-      begin
-        @project.issues.find_by!(iid: params[:id])
-      rescue ActiveRecord::RecordNotFound
-        redirect_old
-      end
+    @noteable = @issue ||= @project.issues.find_by(iid: params[:id]) || redirect_old
   end
   alias_method :subscribable_resource, :issue
   alias_method :issuable, :issue
@@ -225,7 +220,6 @@ class Projects::IssuesController < Projects::ApplicationController
 
     if issue
       redirect_to issue_path(issue)
-      return
     else
       raise ActiveRecord::RecordNotFound.new
     end

app/services/notes/create_service.rb

@@ -15,20 +15,29 @@ 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)
-      content, command_params = slash_commands_service.extract_commands(note)
 
-      note.note = content
+      if slash_commands_service.supported?(note)
+        content, command_params = slash_commands_service.extract_commands(note)
 
-      if note.save
+        only_commands = content.empty?
+
+        note.note = content
+      end
+
+      if !only_commands && note.save
         # Finish the harder work in the background
         NewNoteWorker.perform_in(2.seconds, note.id, params)
         todo_service.new_note(note, current_user)
       end
 
-      # We must add the error after we call #save because errors are reset
-      # when #save is called
-      if slash_commands_service.execute(command_params, note) && note.note.blank?
-        note.errors.add(:commands_only, 'Your commands have been executed!')
+      if command_params && command_params.any?
+        slash_commands_service.execute(command_params, note)
+
+        # We must add the error after we call #save because errors are reset
+        # when #save is called
+        if only_commands
+          note.errors.add(:commands_only, 'Your commands have been executed!')
+        end
       end
 
       note

app/services/notes/slash_commands_service.rb

@@ -5,10 +5,13 @@ module Notes
       'MergeRequest' => MergeRequests::UpdateService
     }
 
+    def supported?(note)
+      noteable_update_service(note) &&
+        can?(current_user, :"update_#{note.noteable_type.underscore}", note.noteable)
+    end
+
     def extract_commands(note)
-      @noteable_update_service = UPDATE_SERVICES[note.noteable_type]
-      return [] unless @noteable_update_service
-      return [] unless can?(current_user, :"update_#{note.noteable_type.underscore}", note.noteable)
+      return [note.note, {}] unless supported?(note)
 
       SlashCommands::InterpretService.new(project, current_user).
         execute(note.note, note.noteable)
@@ -16,9 +19,15 @@ module Notes
 
     def execute(command_params, note)
       return if command_params.empty?
+      return unless supported?(note)
+
+      noteable_update_service(note).new(project, current_user, command_params).execute(note.noteable)
+    end
+
+    private
 
-      @noteable_update_service.new(project, current_user, command_params).
-        execute(note.noteable)
+    def noteable_update_service(note)
+      UPDATE_SERVICES[note.noteable_type]
     end
   end
 end

app/services/slash_commands/interpret_service.rb

@@ -2,16 +2,16 @@ module SlashCommands
   class InterpretService < BaseService
     include Gitlab::SlashCommands::Dsl
 
-    attr_reader :noteable
+    attr_reader :issuable
 
     # Takes a text and interpret the commands that are extracted from it.
     # Returns a hash of changes to be applied to a record.
-    def execute(content, noteable)
-      @noteable = noteable
+    def execute(content, issuable)
+      @issuable = issuable
       @updates = {}
 
       opts = {
-        noteable:     noteable,
+        issuable:     issuable,
         current_user: current_user,
         project:      project
       }
@@ -35,23 +35,24 @@ module SlashCommands
     end
 
     desc do
-      "Close this #{noteable.to_ability_name.humanize(capitalize: false)}"
+      "Close this #{issuable.to_ability_name.humanize(capitalize: false)}"
     end
     condition do
-      noteable.persisted? &&
-        noteable.open? &&
-        current_user.can?(:"update_#{noteable.to_ability_name}", noteable)
+      issuable.persisted? &&
+        issuable.open? &&
+        current_user.can?(:"update_#{issuable.to_ability_name}", issuable)
     end
     command :close do
       @updates[:state_event] = 'close'
     end
 
     desc do
-      "Reopen this #{noteable.to_ability_name.humanize(capitalize: false)}"
+      "Reopen this #{issuable.to_ability_name.humanize(capitalize: false)}"
     end
     condition do
-      noteable.closed? &&
-        current_user.can?(:"update_#{noteable.to_ability_name}", noteable)
+      issuable.persisted? &&
+        issuable.closed? &&
+        current_user.can?(:"update_#{issuable.to_ability_name}", issuable)
     end
     command :reopen, :open do
       @updates[:state_event] = 'reopen'
@@ -60,8 +61,8 @@ module SlashCommands
     desc 'Change title'
     params '<New title>'
     condition do
-      noteable.persisted? &&
-        current_user.can?(:"update_#{noteable.to_ability_name}", noteable)
+      issuable.persisted? &&
+        current_user.can?(:"update_#{issuable.to_ability_name}", issuable)
     end
     command :title do |title_param|
       @updates[:title] = title_param
@@ -70,7 +71,7 @@ module SlashCommands
     desc 'Assign'
     params '@user'
     condition do
-      current_user.can?(:"admin_#{noteable.to_ability_name}", project)
+      current_user.can?(:"admin_#{issuable.to_ability_name}", project)
     end
     command :assign do |assignee_param|
       user = extract_references(assignee_param, :user).first
@@ -82,8 +83,9 @@ module SlashCommands
 
     desc 'Remove assignee'
     condition do
-      noteable.assignee_id? &&
-        current_user.can?(:"admin_#{noteable.to_ability_name}", project)
+      issuable.persisted? &&
+        issuable.assignee_id? &&
+        current_user.can?(:"admin_#{issuable.to_ability_name}", project)
     end
     command :unassign, :remove_assignee do
       @updates[:assignee_id] = nil
@@ -92,7 +94,7 @@ module SlashCommands
     desc 'Set milestone'
     params '%"milestone"'
     condition do
-      current_user.can?(:"admin_#{noteable.to_ability_name}", project) &&
+      current_user.can?(:"admin_#{issuable.to_ability_name}", project) &&
         project.milestones.active.any?
     end
     command :milestone do |milestone_param|
@@ -104,8 +106,9 @@ module SlashCommands
 
     desc 'Remove milestone'
     condition do
-      noteable.milestone_id? &&
-        current_user.can?(:"admin_#{noteable.to_ability_name}", project)
+      issuable.persisted? &&
+        issuable.milestone_id? &&
+        current_user.can?(:"admin_#{issuable.to_ability_name}", project)
     end
     command :clear_milestone, :remove_milestone do
       @updates[:milestone_id] = nil
@@ -114,7 +117,7 @@ module SlashCommands
     desc 'Add label(s)'
     params '~label1 ~"label 2"'
     condition do
-      current_user.can?(:"admin_#{noteable.to_ability_name}", project) &&
+      current_user.can?(:"admin_#{issuable.to_ability_name}", project) &&
         project.labels.any?
     end
     command :label, :labels do |labels_param|
@@ -126,8 +129,9 @@ module SlashCommands
     desc 'Remove label(s)'
     params '~label1 ~"label 2"'
     condition do
-      noteable.labels.any? &&
-        current_user.can?(:"admin_#{noteable.to_ability_name}", project)
+      issuable.persisted? &&
+        issuable.labels.any? &&
+        current_user.can?(:"admin_#{issuable.to_ability_name}", project)
     end
     command :unlabel, :remove_label, :remove_labels do |labels_param|
       label_ids = find_label_ids(labels_param)
@@ -137,8 +141,9 @@ module SlashCommands
 
     desc 'Remove all labels'
     condition do
-      noteable.labels.any? &&
-        current_user.can?(:"admin_#{noteable.to_ability_name}", project)
+      issuable.persisted? &&
+        issuable.labels.any? &&
+        current_user.can?(:"admin_#{issuable.to_ability_name}", project)
     end
     command :clear_labels, :clear_label do
       @updates[:label_ids] = []
@@ -146,8 +151,8 @@ module SlashCommands
 
     desc 'Add a todo'
     condition do
-      noteable.persisted? &&
-        !TodoService.new.todo_exist?(noteable, current_user)
+      issuable.persisted? &&
+        !TodoService.new.todo_exist?(issuable, current_user)
     end
     command :todo do
       @updates[:todo_event] = 'add'
@@ -155,7 +160,8 @@ module SlashCommands
 
     desc 'Mark todo as done'
     condition do
-        TodoService.new.todo_exist?(noteable, current_user)
+      issuable.persisted? &&
+        TodoService.new.todo_exist?(issuable, current_user)
     end
     command :done do
       @updates[:todo_event] = 'done'
@@ -163,8 +169,8 @@ module SlashCommands
 
     desc 'Subscribe'
     condition do
-      noteable.persisted? &&
-        !noteable.subscribed?(current_user)
+      issuable.persisted? &&
+        !issuable.subscribed?(current_user)
     end
     command :subscribe do
       @updates[:subscription_event] = 'subscribe'
@@ -172,8 +178,8 @@ module SlashCommands
 
     desc 'Unsubscribe'
     condition do
-      noteable.persisted? &&
-        noteable.subscribed?(current_user)
+      issuable.persisted? &&
+        issuable.subscribed?(current_user)
     end
     command :unsubscribe do
       @updates[:subscription_event] = 'unsubscribe'
@@ -182,8 +188,8 @@ module SlashCommands
     desc 'Set due date'
     params '<in 2 days; this Friday; December 31st>'
     condition do
-      noteable.respond_to?(:due_date) &&
-        current_user.can?(:"update_#{noteable.to_ability_name}", noteable)
+      issuable.respond_to?(:due_date) &&
+        current_user.can?(:"update_#{issuable.to_ability_name}", issuable)
     end
     command :due, :due_date do |due_date_param|
       due_date = Chronic.parse(due_date_param).try(:to_date)
@@ -193,9 +199,10 @@ module SlashCommands
 
     desc 'Remove due date'
     condition do
-      noteable.respond_to?(:due_date) &&
-        noteable.due_date? &&
-        current_user.can?(:"update_#{noteable.to_ability_name}", noteable)
+      issuable.persisted? &&
+        issuable.respond_to?(:due_date) &&
+        issuable.due_date? &&
+        current_user.can?(:"update_#{issuable.to_ability_name}", issuable)
     end
     command :clear_due_date do
       @updates[:due_date] = nil

lib/gitlab/slash_commands/command_definition.rb

@@ -3,8 +3,8 @@ module Gitlab
     class CommandDefinition
       attr_accessor :name, :aliases, :description, :params, :condition_block, :action_block
 
-      def valid?
-        name.present?
+      def initialize(name)
+        @name = name
       end
 
       def all_names
@@ -22,13 +22,6 @@ module Gitlab
         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)
 

lib/gitlab/slash_commands/dsl.rb

@@ -73,16 +73,13 @@ module Gitlab
         def command(*command_names, &block)
           name, *aliases = command_names
 
-          definition = CommandDefinition.new
-          definition.name            =  name
+          definition = CommandDefinition.new(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|

lib/gitlab/slash_commands/extractor.rb

@@ -29,8 +29,8 @@ module Gitlab
       # commands = extractor.extract_commands(msg) #=> [['labels', '~foo ~"bar baz"']]
       # msg #=> "hello\nworld"
       # ```
-      def extract_commands(content, opts)
-        return [] unless content
+      def extract_commands(content, opts = {})
+        return [content, []] unless content
 
         content = content.dup
 
@@ -107,7 +107,13 @@ module Gitlab
               # Command not in a blockquote, blockcode, or HTML tag:
               # /close
 
-              ^\/(?<cmd>#{Regexp.union(names)})(?:$|\ (?<args>[^\/\n]*)$)
+              ^\/
+              (?<cmd>#{Regexp.union(names)})
+              (?:
+                [ ]
+                (?<args>[^\/\n]*)
+              )?
+              (?:\n|$)
             )
         }mx
       end

spec/lib/gitlab/slash_commands/command_definition_spec.rb

+require 'spec_helper'
+
+describe Gitlab::SlashCommands::CommandDefinition do
+  subject { described_class.new(:command) }
+
+  describe "#all_names" do
+    context "when the command has aliases" do
+      before do
+        subject.aliases = [:alias1, :alias2]
+      end
+
+      it "returns an array with the name and aliases" do
+        expect(subject.all_names).to eq([:command, :alias1, :alias2])
+      end
+    end
+
+    context "when the command doesn't have aliases" do
+      it "returns an array with the name" do
+        expect(subject.all_names).to eq([:command])
+      end
+    end
+  end
+
+  describe "#noop?" do
+    context "when the command has an action block" do
+      before do
+        subject.action_block = -> { }
+      end
+
+      it "returns false" do
+        expect(subject.noop?).to be false
+      end
+    end
+
+    context "when the command doesn't have an action block" do
+      it "returns true" do
+        expect(subject.noop?).to be true
+      end
+    end
+  end
+
+  describe "#available?" do
+    let(:opts) { { go: false } }
+
+    context "when the command has a condition block" do
+      before do
+        subject.condition_block = -> { go }
+      end
+
+      context "when the condition block returns true" do
+        before do
+          opts[:go] = true
+        end
+
+        it "returns true" do
+          expect(subject.available?(opts)).to be true
+        end
+      end
+
+      context "when the condition block returns false" do
+        it "returns false" do
+          expect(subject.available?(opts)).to be false
+        end
+      end
+    end
+
+    context "when the command doesn't have a condition block" do
+      it "returns true" do
+        expect(subject.available?(opts)).to be true
+      end
+    end
+  end
+
+  describe "#execute" do
+    let(:context) { OpenStruct.new(run: false) }
+
+    context "when the command is a noop" do
+      it "doesn't execute the command" do
+        expect(context).not_to receive(:instance_exec)
+
+        subject.execute(context, {})
+
+        expect(context.run).to be false
+      end
+    end
+
+    context "when the command is not a noop" do
+      before do
+        subject.action_block = -> { self.run = true }
+      end
+
+      context "when the command is not available" do
+        before do
+          subject.condition_block = -> { false }
+        end
+
+        it "doesn't execute the command" do
+          subject.execute(context, {})
+
+          expect(context.run).to be false
+        end
+      end
+
+      context "when the command is available" do
+        context "when the command has an exact number of arguments" do
+          before do
+            subject.action_block = ->(arg) { self.run = arg }
+          end
+
+          context "when the command is provided a wrong number of arguments" do
+            it "doesn't execute the command" do
+              subject.execute(context, {}, true, true)
+
+              expect(context.run).to be false
+            end
+          end
+
+          context "when the command is provided the right number of arguments" do
+            it "executes the command" do
+              subject.execute(context, {}, true)
+
+              expect(context.run).to be true
+            end
+          end
+        end
+
+        context "when the command has a variable number of arguments" do
+          before do
+            subject.action_block = ->(*args) { self.run = args.first }
+          end
+
+          context "when the command is provided any number of arguments" do
+            it "executes the command" do
+              subject.execute(context, {}, true, true)
+
+              expect(context.run).to be true
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/slash_commands/dsl_spec.rb

@@ -10,9 +10,9 @@ describe Gitlab::SlashCommands::Dsl do
         "Hello World!"
       end
 
-      desc 'A command returning a value'
+      desc { "A command with #{something}" }
       command :returning do
-        return 42
+        42
       end
 
       params 'The first argument'
@@ -28,7 +28,7 @@ describe Gitlab::SlashCommands::Dsl do
         [arg1, arg2]
       end
 
-      command :cc, noop: true
+      command :cc
 
       condition do
         project == 'foo'
@@ -49,182 +49,74 @@ describe Gitlab::SlashCommands::Dsl do
         {
           name: :no_args, aliases: [:none],
           description: 'A command with no args', params: [],
-          condition_block: nil, action_block: a_kind_of(Proc),
-          opts: {}
+          condition_block: nil, action_block: a_kind_of(Proc)
         },
         {
           name: :returning, aliases: [],
           description: 'A command returning a value', params: [],
-          condition_block: nil, action_block: a_kind_of(Proc),
-          opts: {}
+          condition_block: nil, action_block: a_kind_of(Proc)
         },
         {
           name: :one_arg, aliases: [:once, :first],
           description: '', params: ['The first argument'],
-          condition_block: nil, action_block: a_kind_of(Proc),
-          opts: {}
+          condition_block: nil, action_block: a_kind_of(Proc)
         },
         {
           name: :two_args, aliases: [],
           description: '', params: ['The first argument', 'The second argument'],
-          condition_block: nil, action_block: a_kind_of(Proc),
-          opts: {}
+          condition_block: nil, action_block: a_kind_of(Proc)
         },
         {
           name: :cc, aliases: [],
           description: '', params: [],
-          condition_block: nil, action_block: nil,
-          opts: { noop: true }
+          condition_block: nil, action_block: nil
         },
         {
           name: :wildcard, aliases: [],
           description: '', params: [],
-          condition_block: nil, action_block: a_kind_of(Proc),
-          opts: {}
+          condition_block: nil, action_block: a_kind_of(Proc)
         }
       ]
     end
 
     it 'returns an array with commands definitions' do
-      expect(DummyClass.command_definitions).to match_array base_expected
-    end
-
-    context 'with options passed' do
-      context 'when condition is met' do
-        let(:expected) do
-          base_expected << {
-            name: :cond_action, aliases: [],
-            description: '', params: [],
-            condition_block: a_kind_of(Proc), action_block: a_kind_of(Proc),
-            opts: {}
-          }
-        end
-
-        it 'returns an array with commands definitions' do
-          expect(DummyClass.command_definitions(project: 'foo')).to match_array expected
-        end
-      end
-
-      context 'when condition is not met' do
-        it 'returns an array with commands definitions without actions that did not met conditions' do
-          expect(DummyClass.command_definitions(project: 'bar')).to match_array base_expected
-        end
-      end
-
-      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'
-
-          expect(DummyClass.command_definitions(noteable: 'merge request')).to match_array base_expected
-        end
-      end
-    end
-  end
-
-  describe '.command_names' do
-    let(:base_expected) do
-      [
-        :no_args, :none, :returning, :one_arg,
-        :once, :first, :two_args, :wildcard
-      ]
-    end
-
-    it 'returns an array with commands definitions' do
-      expect(DummyClass.command_names).to eq base_expected
-    end
-
-    context 'with options passed' do
-      context 'when condition is met' do
-        let(:expected) { base_expected << :cond_action }
-
-        it 'returns an array with commands definitions' do
-          expect(DummyClass.command_names(project: 'foo')).to match_array expected
-        end
-      end
-
-      context 'when condition is not met' do
-        it 'returns an array with commands definitions without action that did not met conditions' do
-          expect(DummyClass.command_names(project: 'bar')).to match_array base_expected
-        end
-      end
-    end
-  end
-
-  let(:dummy) { DummyClass.new(nil) }
-
-  describe '#execute_command' do
-    describe 'command with no args' do
-      context 'called with no args' do
-        it 'succeeds' do
-          expect(dummy.execute_command(:no_args)).to eq 'Hello World!'
-        end
-      end
-    end
-
-    describe 'command with an explicit return' do
-      context 'called with no args' do
-        it 'succeeds' do
-          expect { dummy.execute_command(:returning) }.to raise_error(LocalJumpError)
-        end
-      end
-    end
-
-    describe 'command with one arg' do
-      context 'called with one arg' do
-        it 'succeeds' do
-          expect(dummy.execute_command(:one_arg, 42)).to eq 42
-        end
-      end
-    end
-
-    describe 'command with two args' do
-      context 'called with two args' do
-        it 'succeeds' do
-          expect(dummy.execute_command(:two_args, 42, 'foo')).to eq [42, 'foo']
-        end
-      end
-    end
-
-    describe 'noop command' do
-      it 'returns nil' do
-        expect(dummy.execute_command(:cc)).to be_nil
-      end
-    end
-
-    describe 'command with condition' do
-      context 'when condition is not met' do
-        it 'returns nil' do
-          expect(dummy.execute_command(:cond_action)).to be_nil
-        end
-      end
-
-      context 'when condition is met' do
-        let(:dummy) { DummyClass.new('foo') }
-
-        it 'succeeds' do
-          expect(dummy.execute_command(:cond_action, 42)).to eq 42
-        end
-      end
-    end
-
-    describe 'command with wildcard' do
-      context 'called with no args' do
-        it 'succeeds' do
-          expect(dummy.execute_command(:wildcard)).to eq []
-        end
-      end
-
-      context 'called with one arg' do
-        it 'succeeds' do
-          expect(dummy.execute_command(:wildcard, 42)).to eq [42]
-        end
-      end
-
-      context 'called with two args' do
-        it 'succeeds' do
-          expect(dummy.execute_command(:wildcard, 42, 'foo')).to eq [42, 'foo']
-        end
-      end
+      no_args_def, returning_def, one_arg_def, two_args_def, cc_def, cond_action_def, wildcard_def = DummyClass.command_definitions
+
+      expect(no_args_def.name).to eq(:no_args)
+      expect(no_args_def.aliases).to eq([:none])
+      expect(no_args_def.description).to eq('A command with no args')
+      expect(no_args_def.params).to eq([])
+      expect(no_args_def.condition_block).to be_nil
+      expect(no_args_def.action_block).to be_a_kind_of(Proc)
+
+      expect(returning_def.name).to eq(:returning)
+      expect(returning_def.aliases).to eq([])
+      expect(returning_def.description).to be_a_kind_of(Proc)
+      expect(returning_def.to_h(something: "a block description")[:description]).to eq('A command with a block description')
+      expect(returning_def.params).to eq([])
+      expect(returning_def.condition_block).to be_nil
+      expect(returning_def.action_block).to be_a_kind_of(Proc)
+
+      expect(one_arg_def.name).to eq(:one_arg)
+      expect(one_arg_def.aliases).to eq([:once, :first])
+      expect(one_arg_def.description).to eq('')
+      expect(one_arg_def.params).to eq(['The first argument'])
+      expect(one_arg_def.condition_block).to be_nil
+      expect(one_arg_def.action_block).to be_a_kind_of(Proc)
+
+      expect(cc_def.name).to eq(:cc)
+      expect(cc_def.aliases).to eq([])
+      expect(cc_def.description).to eq('')
+      expect(cc_def.params).to eq([])
+      expect(cc_def.condition_block).to be_nil
+      expect(cc_def.action_block).to be_nil
+
+      expect(wildcard_def.name).to eq(:wildcard)
+      expect(wildcard_def.aliases).to eq([])
+      expect(wildcard_def.description).to eq('')
+      expect(wildcard_def.params).to eq([])
+      expect(wildcard_def.condition_block).to be_nil
+      expect(wildcard_def.action_block).to be_a_kind_of(Proc)
     end
   end
 end

spec/lib/gitlab/slash_commands/extractor_spec.rb

 require 'spec_helper'
 
 describe Gitlab::SlashCommands::Extractor do
-  let(:extractor) { described_class.new([:open, :assign, :labels, :power]) }
+  let(:definitions) do
+    Class.new do
+      include Gitlab::SlashCommands::Dsl
+
+      command(:reopen, :open) { }
+      command(:assign) { }
+      command(:labels) { }
+      command(:power) { }
+    end.command_definitions
+  end
+
+  let(:extractor) { described_class.new(definitions) }
 
   shared_examples 'command with no argument' do
     it 'extracts command' do
-      commands = extractor.extract_commands(original_msg)
+      msg, commands = extractor.extract_commands(original_msg)
 
       expect(commands).to eq [['open']]
-      expect(original_msg).to eq final_msg
+      expect(msg).to eq final_msg
     end
   end
 
   shared_examples 'command with a single argument' do
     it 'extracts command' do
-      commands = extractor.extract_commands(original_msg)
+      msg, commands = extractor.extract_commands(original_msg)
 
       expect(commands).to eq [['assign', '@joe']]
-      expect(original_msg).to eq final_msg
+      expect(msg).to eq final_msg
     end
   end
 
   shared_examples 'command with multiple arguments' do
     it 'extracts command' do
-      commands = extractor.extract_commands(original_msg)
+      msg, commands = extractor.extract_commands(original_msg)
 
       expect(commands).to eq [['labels', '~foo ~"bar baz" label']]
-      expect(original_msg).to eq final_msg
+      expect(msg).to eq final_msg
     end
   end
 
@@ -49,7 +60,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)
+          msg, commands = extractor.extract_commands(msg)
 
           expect(commands).to be_empty
           expect(msg).to eq "hello\nworld /open"
@@ -59,7 +70,7 @@ describe Gitlab::SlashCommands::Extractor do
       context 'at the end of content' do
         it_behaves_like 'command with no argument' do
           let(:original_msg) { "hello\n/open" }
-          let(:final_msg) { "hello\n" }
+          let(:final_msg) { "hello" }
         end
       end
     end
@@ -82,7 +93,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)
+          msg, commands = extractor.extract_commands(msg)
 
           expect(commands).to be_empty
           expect(msg).to eq "hello\nworld /assign @joe"
@@ -92,14 +103,14 @@ describe Gitlab::SlashCommands::Extractor do
       context 'at the end of content' do
         it_behaves_like 'command with a single argument' do
           let(:original_msg) { "hello\n/assign @joe" }
-          let(:final_msg) { "hello\n" }
+          let(:final_msg) { "hello" }
         end
       end
 
       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)
+          msg, commands = extractor.extract_commands(msg)
 
           expect(commands).to be_empty
           expect(msg).to eq "hello\n/assign@joe\nworld"
@@ -125,7 +136,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)
+          msg, commands = extractor.extract_commands(msg)
 
           expect(commands).to be_empty
           expect(msg).to eq %(hello\nworld /labels ~foo ~"bar baz" label)
@@ -135,14 +146,14 @@ describe Gitlab::SlashCommands::Extractor do
       context 'at the end of content' do
         it_behaves_like 'command with multiple arguments' do
           let(:original_msg) { %(hello\n/labels ~foo ~"bar baz" label) }
-          let(:final_msg) { "hello\n" }
+          let(:final_msg) { "hello" }
         end
       end
 
       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)
+          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 +163,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)
+      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,15 +171,15 @@ 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)
+      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"
+      expect(msg).to eq "hello\nworld"
     end
 
     it 'does not alter original content if no command is found' do
       msg = 'Fixes #123'
-      commands = extractor.extract_commands(msg)
+      msg, commands = extractor.extract_commands(msg)
 
       expect(commands).to be_empty
       expect(msg).to eq 'Fixes #123'
@@ -177,7 +188,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)
+      msg, commands = extractor.extract_commands(msg)
 
       expect(commands).to be_empty
       expect(msg).to eq expected
@@ -186,7 +197,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)
+      msg, commands = extractor.extract_commands(msg)
 
       expect(commands).to be_empty
       expect(msg).to eq expected
@@ -195,7 +206,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)
+      msg, commands = extractor.extract_commands(msg)
 
       expect(commands).to be_empty
       expect(msg).to eq expected

spec/services/notes/create_service_spec.rb

@@ -56,7 +56,7 @@ describe Notes::CreateService, services: true do
 
     it "creates regular note if emoji name is invalid" do
       opts = {
-        note: ':smile: moretext: ',
+        note: ':smile: moretext:',
         noteable_type: 'Issue',
         noteable_id: issue.id
       }

spec/services/notes/slash_commands_service_spec.rb

@@ -12,7 +12,6 @@ describe Notes::SlashCommandsService, services: true do
 
     before do
       note.note = note_text
-      described_class.new(project, master).execute(note)
     end
 
     describe 'note with only command' do
@@ -20,7 +19,10 @@ describe Notes::SlashCommandsService, services: true do
         let(:note_text) { %(/close\n/assign @#{assignee.username}") }
 
         it 'saves the note and does not alter the note text' do
-          expect(note.note).to eq note_text
+          content, command_params = service.extract_commands(note)
+
+          expect(content).to eq note_text
+          expect(command_params).to be_empty
         end
       end
     end
@@ -30,7 +32,10 @@ describe Notes::SlashCommandsService, services: true do
         let(:note_text) { %(HELLO\n/close\n/assign @#{assignee.username}\nWORLD) }
 
         it 'saves the note and does not alter the note text' do
-          expect(note.note).to eq note_text
+          content, command_params = service.extract_commands(note)
+
+          expect(content).to eq note_text
+          expect(command_params).to be_empty
         end
       end
     end
@@ -53,9 +58,10 @@ describe Notes::SlashCommandsService, services: true do
         end
 
         it 'closes noteable, sets labels, assigns, and sets milestone to noteable, and leave no note' do
-          described_class.new(project, master).execute(note)
+          content, command_params = service.extract_commands(note)
+          service.execute(command_params, note)
 
-          expect(note.note).to eq ''
+          expect(content).to eq ''
           expect(note.noteable).to be_closed
           expect(note.noteable.labels).to match_array(labels)
           expect(note.noteable.assignee).to eq(assignee)
@@ -71,9 +77,10 @@ describe Notes::SlashCommandsService, services: true do
         let(:note_text) { '/open' }
 
         it 'opens the noteable, and leave no note' do
-          described_class.new(project, master).execute(note)
+          content, command_params = service.extract_commands(note)
+          service.execute(command_params, note)
 
-          expect(note.note).to eq ''
+          expect(content).to eq ''
           expect(note.noteable).to be_open
         end
       end
@@ -86,9 +93,10 @@ describe Notes::SlashCommandsService, services: true do
         end
 
         it 'closes noteable, sets labels, assigns, and sets milestone to noteable' do
-          described_class.new(project, master).execute(note)
+          content, command_params = service.extract_commands(note)
+          service.execute(command_params, note)
 
-          expect(note.note).to eq "HELLO\nWORLD"
+          expect(content).to eq "HELLO\nWORLD"
           expect(note.noteable).to be_closed
           expect(note.noteable.labels).to match_array(labels)
           expect(note.noteable.assignee).to eq(assignee)
@@ -104,9 +112,10 @@ describe Notes::SlashCommandsService, services: true do
         let(:note_text) { "HELLO\n/open\nWORLD" }
 
         it 'opens the noteable' do
-          described_class.new(project, master).execute(note)
+          content, command_params = service.extract_commands(note)
+          service.execute(command_params, note)
 
-          expect(note.note).to eq "HELLO\nWORLD"
+          expect(content).to eq "HELLO\nWORLD"
           expect(note.noteable).to be_open
         end
       end
@@ -114,6 +123,8 @@ describe Notes::SlashCommandsService, services: true do
   end
 
   describe '#execute' do
+    let(:service) { described_class.new(project, master) }
+
     it_behaves_like 'note on noteable that supports slash commands' do
       let(:note) { build(:note_on_issue, project: project) }
     end

spec/services/slash_commands/interpret_service_spec.rb

@@ -12,141 +12,6 @@ describe SlashCommands::InterpretService, services: true do
     project.team << [user, :developer]
   end
 
-  describe '#command_names' do
-    subject do
-      described_class.command_names(
-        project: project,
-        noteable: issue,
-        current_user: user
-      )
-    end
-
-    it 'returns the basic known commands' do
-      is_expected.to match_array([
-        :close,
-        :title,
-        :assign, :reassign,
-        :todo,
-        :subscribe,
-        :due_date, :due
-      ])
-    end
-
-    context 'when noteable is open' do
-      it 'includes the :close command' do
-        is_expected.to include(*[:close])
-      end
-    end
-
-    context 'when noteable is closed' do
-      before do
-        issue.close!
-      end
-
-      it 'includes the :open, :reopen commands' do
-        is_expected.to include(*[:open, :reopen])
-      end
-    end
-
-    context 'when noteable has an assignee' do
-      before do
-        issue.update(assignee_id: user.id)
-      end
-
-      it 'includes the :unassign, :remove_assignee commands' do
-        is_expected.to include(*[:unassign, :remove_assignee])
-      end
-    end
-
-    context 'when noteable has a milestone' do
-      before do
-        issue.update(milestone: milestone)
-      end
-
-      it 'includes the :clear_milestone, :remove_milestone commands' do
-        is_expected.to include(*[:milestone, :clear_milestone, :remove_milestone])
-      end
-    end
-
-    context 'when project has a milestone' do
-      before do
-        milestone
-      end
-
-      it 'includes the :milestone command' do
-        is_expected.to include(*[:milestone])
-      end
-    end
-
-    context 'when noteable has a label' do
-      before do
-        issue.update(label_ids: [bug.id])
-      end
-
-      it 'includes the :unlabel, :remove_labels, :remove_label, :clear_labels, :clear_label commands' do
-        is_expected.to include(*[:unlabel, :remove_labels, :remove_label, :clear_labels, :clear_label])
-      end
-    end
-
-    context 'when project has a label' do
-      before do
-        inprogress
-      end
-
-      it 'includes the :labels, :label commands' do
-        is_expected.to include(*[:labels, :label])
-      end
-    end
-
-    context 'when user has no todo' do
-      it 'includes the :todo command' do
-        is_expected.to include(*[:todo])
-      end
-    end
-
-    context 'when user has a todo' do
-      before do
-        TodoService.new.mark_todo(issue, user)
-      end
-
-      it 'includes the :done command' do
-        is_expected.to include(*[:done])
-      end
-    end
-
-    context 'when user is not subscribed' do
-      it 'includes the :subscribe command' do
-        is_expected.to include(*[:subscribe])
-      end
-    end
-
-    context 'when user is subscribed' do
-      before do
-        issue.subscribe(user)
-      end
-
-      it 'includes the :unsubscribe command' do
-        is_expected.to include(*[:unsubscribe])
-      end
-    end
-
-    context 'when noteable has a no due date' do
-      it 'includes the :due_date, :due commands' do
-        is_expected.to include(*[:due_date, :due])
-      end
-    end
-
-    context 'when noteable has a due date' do
-      before do
-        issue.update(due_date: Date.today)
-      end
-
-      it 'includes the :clear_due_date command' do
-        is_expected.to include(*[:due_date, :due, :clear_due_date])
-      end
-    end
-  end
-
   describe '#execute' do
     let(:service) { described_class.new(project, user) }
     let(:merge_request) { create(:merge_request, source_project: project) }
@@ -154,60 +19,60 @@ describe SlashCommands::InterpretService, services: true do
     shared_examples 'open command' do
       it 'returns state_event: "open" if content contains /open' do
         issuable.close!
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(state_event: 'reopen')
+        expect(updates).to eq(state_event: 'reopen')
       end
     end
 
     shared_examples 'close command' do
       it 'returns state_event: "close" if content contains /open' do
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(state_event: 'close')
+        expect(updates).to eq(state_event: 'close')
       end
     end
 
     shared_examples 'title command' do
       it 'populates title: "A brand new title" if content contains /title A brand new title' do
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(title: 'A brand new title')
+        expect(updates).to eq(title: 'A brand new title')
       end
     end
 
     shared_examples 'assign command' do
       it 'fetches assignee and populates assignee_id if content contains /assign' do
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(assignee_id: user.id)
+        expect(updates).to eq(assignee_id: user.id)
       end
     end
 
     shared_examples 'unassign command' do
       it 'populates assignee_id: nil if content contains /unassign' do
         issuable.update(assignee_id: user.id)
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(assignee_id: nil)
+        expect(updates).to eq(assignee_id: nil)
       end
     end
 
     shared_examples 'milestone command' do
       it 'fetches milestone and populates milestone_id if content contains /milestone' do
         milestone # populate the milestone
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(milestone_id: milestone.id)
+        expect(updates).to eq(milestone_id: milestone.id)
       end
     end
 
     shared_examples 'clear_milestone command' do
       it 'populates milestone_id: nil if content contains /clear_milestone' do
         issuable.update(milestone_id: milestone.id)
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(milestone_id: nil)
+        expect(updates).to eq(milestone_id: nil)
       end
     end
 
@@ -215,86 +80,86 @@ describe SlashCommands::InterpretService, services: true do
       it 'fetches label ids and populates add_label_ids if content contains /label' do
         bug # populate the label
         inprogress # populate the label
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(add_label_ids: [bug.id, inprogress.id])
+        expect(updates).to eq(add_label_ids: [bug.id, inprogress.id])
       end
     end
 
     shared_examples 'unlabel command' do
       it 'fetches label ids and populates remove_label_ids if content contains /unlabel' do
         issuable.update(label_ids: [inprogress.id]) # populate the label
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(remove_label_ids: [inprogress.id])
+        expect(updates).to eq(remove_label_ids: [inprogress.id])
       end
     end
 
     shared_examples 'clear_labels command' do
       it 'populates label_ids: [] if content contains /clear_labels' do
         issuable.update(label_ids: [inprogress.id]) # populate the label
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(label_ids: [])
+        expect(updates).to eq(label_ids: [])
       end
     end
 
     shared_examples 'todo command' do
       it 'populates todo_event: "add" if content contains /todo' do
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(todo_event: 'add')
+        expect(updates).to eq(todo_event: 'add')
       end
     end
 
     shared_examples 'done command' do
       it 'populates todo_event: "done" if content contains /done' do
         TodoService.new.mark_todo(issuable, user)
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(todo_event: 'done')
+        expect(updates).to eq(todo_event: 'done')
       end
     end
 
     shared_examples 'subscribe command' do
       it 'populates subscription_event: "subscribe" if content contains /subscribe' do
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(subscription_event: 'subscribe')
+        expect(updates).to eq(subscription_event: 'subscribe')
       end
     end
 
     shared_examples 'unsubscribe command' do
       it 'populates subscription_event: "unsubscribe" if content contains /unsubscribe' do
         issuable.subscribe(user)
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(subscription_event: 'unsubscribe')
+        expect(updates).to eq(subscription_event: 'unsubscribe')
       end
     end
 
     shared_examples 'due_date command' do
       it 'populates due_date: Date.new(2016, 8, 28) if content contains /due_date 2016-08-28' do
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(due_date: defined?(expected_date) ? expected_date : Date.new(2016, 8, 28))
+        expect(updates).to eq(due_date: defined?(expected_date) ? expected_date : Date.new(2016, 8, 28))
       end
     end
 
     shared_examples 'clear_due_date command' do
       it 'populates due_date: nil if content contains /clear_due_date' do
         issuable.update(due_date: Date.today)
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to eq(due_date: nil)
+        expect(updates).to eq(due_date: nil)
       end
     end
 
     shared_examples 'empty command' do
       it 'populates {} if content contains an unsupported command' do
-        changes = service.execute(content, issuable)
+        _, updates = service.execute(content, issuable)
 
-        expect(changes).to be_empty
+        expect(updates).to be_empty
       end
     end