Simplify the slash commands DSL to store action blocks instead of creating methods

Other improvements:
- Ensure slash commands autocomplete doesn't break when noteable_type is not given
- Slash commands: improve autocomplete behavior and /due command
- We don't display slash commands for note edit forms.
- Add tests for reply by email with slash commands
- Be sure to execute slash commands after the note creation in Notes::CreateService

Signed-off-by: Rémy Coutable <remy@rymai.me>

2 files changed, 135 insertions, 55 deletions

diff --git a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
index afb072105cf..4909fed6b77 100644
--- a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
+++ b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
@@ -75,13 +75,54 @@ describe Gitlab::Email::Handler::CreateNoteHandler, lib: true do
           project.team << [user, :developer]
         end
 
-        it 'raises a CommandsOnlyNoteError' do
-          expect { receiver.execute }.not_to raise_error
+        it 'does not raise an error' do
+          expect(TodoService.new.todo_exist?(noteable, user)).to be_falsy
+
+          # One system note is created for the 'close' event
+          expect { receiver.execute }.to change { noteable.notes.count }.by(1)
+
+          expect(noteable.reload).to be_closed
+          expect(noteable.due_date).to eq(Date.tomorrow)
+          expect(TodoService.new.todo_exist?(noteable, user)).to be_truthy
         end
       end
     end
   end
 
+  context 'when the note contains slash commands' do
+    let!(:email_raw) { fixture_file("emails/commands_in_reply.eml") }
+
+    context 'and current user cannot update noteable' do
+      it 'post a note and does not update the noteable' do
+        expect(TodoService.new.todo_exist?(noteable, user)).to be_falsy
+
+        # One system note is created for the new note
+        expect { receiver.execute }.to change { noteable.notes.count }.by(1)
+
+        expect(noteable.reload).to be_open
+        expect(noteable.due_date).to be_nil
+        expect(TodoService.new.todo_exist?(noteable, user)).to be_falsy
+      end
+    end
+
+    context 'and current user can update noteable' do
+      before do
+        project.team << [user, :developer]
+      end
+
+      it 'post a note and updates the noteable' do
+        expect(TodoService.new.todo_exist?(noteable, user)).to be_falsy
+
+        # One system note is created for the new note, one for the 'close' event
+        expect { receiver.execute }.to change { noteable.notes.count }.by(2)
+
+        expect(noteable.reload).to be_closed
+        expect(noteable.due_date).to eq(Date.tomorrow)
+        expect(TodoService.new.todo_exist?(noteable, user)).to be_truthy
+      end
+    end
+  end
+
   context "when the reply is blank" do
     let!(:email_raw) { fixture_file("emails/no_content_reply.eml") }
 
diff --git a/spec/lib/gitlab/slash_commands/dsl_spec.rb b/spec/lib/gitlab/slash_commands/dsl_spec.rb
index 385f534ad6f..500ff3ca1fe 100644
--- a/spec/lib/gitlab/slash_commands/dsl_spec.rb
+++ b/spec/lib/gitlab/slash_commands/dsl_spec.rb
@@ -46,12 +46,42 @@ describe Gitlab::SlashCommands::Dsl do
   describe '.command_definitions' do
     let(:base_expected) do
       [
-        { 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 }
+        {
+          name: :no_args, aliases: [:none],
+          description: 'A command with no args', params: [],
+          condition_block: nil, action_block: a_kind_of(Proc),
+          opts: {}
+        },
+        {
+          name: :returning, aliases: [],
+          description: 'A command returning a value', params: [],
+          condition_block: nil, action_block: a_kind_of(Proc),
+          opts: {}
+        },
+        {
+          name: :one_arg, aliases: [:once, :first],
+          description: '', params: ['The first argument'],
+          condition_block: nil, action_block: a_kind_of(Proc),
+          opts: {}
+        },
+        {
+          name: :two_args, aliases: [],
+          description: '', params: ['The first argument', 'The second argument'],
+          condition_block: nil, action_block: a_kind_of(Proc),
+          opts: {}
+        },
+        {
+          name: :cc, aliases: [],
+          description: '', params: [],
+          condition_block: nil, action_block: nil,
+          opts: { noop: true }
+        },
+        {
+          name: :wildcard, aliases: [],
+          description: '', params: [],
+          condition_block: nil, action_block: a_kind_of(Proc),
+          opts: {}
+        }
       ]
     end
 
@@ -61,7 +91,14 @@ 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: [], noop: false, cond_block: a_kind_of(Proc) } }
+        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
@@ -115,76 +152,78 @@ describe Gitlab::SlashCommands::Dsl do
 
   let(:dummy) { DummyClass.new(nil) }
 
-  describe 'command with no args' do
-    context 'called with no args' do
-      it 'succeeds' do
-        expect(dummy.__send__(:no_args)).to eq 'Hello World!'
+  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
-  end
 
-  describe 'command with an explicit return' do
-    context 'called with no args' do
-      it 'succeeds' do
-        expect(dummy.__send__(:returning)).to eq 42
+    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
-  end
 
-  describe 'command with one arg' do
-    context 'called with one arg' do
-      it 'succeeds' do
-        expect(dummy.__send__(:one_arg, 42)).to eq 42
+    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
-  end
 
-  describe 'command with two args' do
-    context 'called with two args' do
-      it 'succeeds' do
-        expect(dummy.__send__(:two_args, 42, 'foo')).to eq [42, 'foo']
+    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
-  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
+    describe 'noop command' do
       it 'returns nil' do
-        expect(dummy.__send__(:cond_action)).to be_nil
+        expect(dummy.execute_command(:cc)).to be_nil
       end
     end
 
-    context 'when condition is met' do
-      let(:dummy) { DummyClass.new('foo') }
+    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
 
-      it 'succeeds' do
-        expect(dummy.__send__(:cond_action, 42)).to eq 42
+      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
-  end
 
-  describe 'command with wildcard' do
-    context 'called with no args' do
-      it 'succeeds' do
-        expect(dummy.__send__(:wildcard)).to eq []
+    describe 'command with wildcard' do
+      context 'called with no args' do
+        it 'succeeds' do
+          expect(dummy.execute_command(:wildcard)).to eq []
+        end
       end
-    end
 
-    context 'called with one arg' do
-      it 'succeeds' do
-        expect(dummy.__send__(:wildcard, 42)).to eq [42]
+      context 'called with one arg' do
+        it 'succeeds' do
+          expect(dummy.execute_command(:wildcard, 42)).to eq [42]
+        end
       end
-    end
 
-    context 'called with two args' do
-      it 'succeeds' do
-        expect(dummy.__send__(:wildcard, 42, 'foo')).to eq [42, 'foo']
+      context 'called with two args' do
+        it 'succeeds' do
+          expect(dummy.execute_command(:wildcard, 42, 'foo')).to eq [42, 'foo']
+        end
       end
     end
   end