Don't extract slash commands inside blockcode, blockquote or HTML tags

Improve slash command descriptions, support /due tomorrow

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

app/services/issuable_base_service.rb

@@ -173,7 +173,7 @@ class IssuableBaseService < BaseService
   def change_todo(issuable)
     case params.delete(:todo_event)
-    when 'mark'
+    when 'add'
       todo_service.mark_todo(issuable, current_user)
     when 'done'
       todo = TodosFinder.new(current_user).execute.find_by(target: issuable)

app/services/slash_commands/interpret_service.rb

@@ -40,7 +40,7 @@ module SlashCommands
       @updates[:title] = title_param
     end
-    desc 'Reassign'
+    desc 'Assign'
     params '@user'
     command :assign, :reassign do |assignee_param|
       user = extract_references(assignee_param, :user).first
@@ -54,7 +54,7 @@ module SlashCommands
       @updates[:assignee_id] = nil
     end
-    desc 'Change milestone'
+    desc 'Set milestone'
     params '%"milestone"'
     command :milestone do |milestone_param|
       milestone = extract_references(milestone_param, :milestone).first
@@ -93,7 +93,7 @@ module SlashCommands
     desc 'Add a todo'
     command :todo do
-      @updates[:todo_event] = 'mark'
+      @updates[:todo_event] = 'add'
     end
     desc 'Mark todo as done'
@@ -113,11 +113,15 @@ module SlashCommands
     desc 'Set a due date'
     params '<YYYY-MM-DD> | <N days>'
-    command :due_date do |due_date_param|
+    command :due_date, :due do |due_date_param|
       return unless noteable.respond_to?(:due_date)
       due_date = begin
-        Time.now + ChronicDuration.parse(due_date_param)
+        if due_date_param.downcase == 'tomorrow'
+          Date.tomorrow
+        else
+          Time.now + ChronicDuration.parse(due_date_param)
+        end
       rescue ChronicDuration::DurationParseError
         Date.parse(due_date_param) rescue nil
       end

doc/workflow/slash_commands.md

@@ -14,9 +14,9 @@ do.
 | `/close`                   | None                | Close the issue or merge request |
 | `/open`                    | `/reopen`           | Reopen the issue or merge request |
 | `/title <New title>`       | None                | Change title |
-| `/assign @username`        | `/reassign`         | Reassign |
+| `/assign @username`        | `/reassign`         | Assign |
 | `/unassign`                | `/remove_assignee`  | Remove assignee |
-| `/milestone %milestone`    | None                | Change milestone |
+| `/milestone %milestone`    | None                | Set milestone |
 | `/clear_milestone`         | `/remove_milestone` | Remove milestone |
 | `/label ~foo ~"bar baz"`   | `/labels`           | Add label(s) |
 | `/unlabel ~foo ~"bar baz"` | `/remove_label`, `remove_labels` | Remove label(s) |
@@ -25,5 +25,5 @@ do.
 | `/done`                    | None                | Mark todo as done |
 | `/subscribe`               | None                | Subscribe |
 | `/unsubscribe`             | None                | Unsubscribe |
-| `/due_date <YYYY-MM-DD> | <N days>` | None       | Set a due date |
+| `/due_date <YYYY-MM-DD> | <N days>` | `/due`     | Set a due date |
 | `/clear_due_date`          | None                | Remove due date |

lib/gitlab/slash_commands/extractor.rb

@@ -34,9 +34,14 @@ module Gitlab
         commands = []
+        content.delete!("\r")
         content.gsub!(commands_regex) do
-          commands << [$1, $2].flatten.reject(&:blank?)
-          ''
+          if $~[:cmd]
+            commands << [$~[:cmd], $~[:args]].reject(&:blank?)
+            ''
+          else
+            $~[0]
+          end
         end
         commands
@@ -52,7 +57,47 @@ module Gitlab
       #
       #   /^\/(?<cmd>close|reopen|...)(?:( |$))(?<args>[^\/\n]*)(?:\n|$)/
       def commands_regex
-        /^\/(?<cmd>#{command_names.join('|')})(?:( |$))(?<args>[^\/\n]*)(?:\n|$)/
+        @commands_regex ||= %r{
+            (?<code>
+              # Code blocks:
+              # ```
+              # Anything, including `/cmd args` which are ignored by this filter
+              # ```
+
+              ^```
+              .+?
+              \n```$
+            )
+          |
+            (?<html>
+              # HTML block:
+              # <tag>
+              # Anything, including `/cmd args` which are ignored by this filter
+              # </tag>
+
+              ^<[^>]+?>\n
+              .+?
+              \n<\/[^>]+?>$
+            )
+          |
+            (?<html>
+              # Quote block:
+              # >>>
+              # Anything, including `/cmd args` which are ignored by this filter
+              # >>>
+
+              ^>>>
+              .+?
+              \n>>>$
+            )
+          |
+            (?:
+              # Command not in a blockquote, blockcode, or HTML tag:
+              # /close
+
+              ^\/(?<cmd>#{command_names.join('|')})(?:(\ |$))(?<args>[^\/\n]*)(?:\n|$)
+            )
+        }mx
       end
     end
   end

spec/lib/gitlab/slash_commands/extractor_spec.rb

@@ -173,5 +173,32 @@ describe Gitlab::SlashCommands::Extractor do
       expect(commands).to be_empty
       expect(msg).to eq 'Fixes #123'
     end
+
+    it 'does not extract commands inside a blockcode' do
+      msg = 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)
+
+      expect(commands).to be_empty
+      expect(msg).to eq expected
+    end
+
+    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)
+
+      expect(commands).to be_empty
+      expect(msg).to eq expected
+    end
+
+    it 'does not extract commands inside a HTML tag' do
+      msg = 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)
+
+      expect(commands).to be_empty
+      expect(msg).to eq expected
+    end
   end
 end

spec/services/slash_commands/interpret_service_spec.rb

@@ -27,7 +27,7 @@ describe SlashCommands::InterpretService, services: true do
         :done,
         :subscribe,
         :unsubscribe,
-        :due_date,
+        :due_date, :due,
         :clear_due_date
       ])
     end
@@ -119,10 +119,10 @@ describe SlashCommands::InterpretService, services: true do
     end
     shared_examples 'todo command' do
-      it 'populates todo_event: "mark" if content contains /todo' do
+      it 'populates todo_event: "add" if content contains /todo' do
         changes = service.execute(content, issuable)
-        expect(changes).to eq(todo_event: 'mark')
+        expect(changes).to eq(todo_event: 'add')
       end
     end
@@ -154,7 +154,7 @@ describe SlashCommands::InterpretService, services: true do
       it 'populates due_date: Date.new(2016, 8, 28) if content contains /due_date 2016-08-28' do
         changes = service.execute(content, issuable)
-        expect(changes).to eq(due_date: Date.new(2016, 8, 28))
+        expect(changes).to eq(due_date: defined?(expected_date) ? expected_date : Date.new(2016, 8, 28))
       end
     end
@@ -369,6 +369,12 @@ describe SlashCommands::InterpretService, services: true do
       let(:issuable) { issue }
     end
+    it_behaves_like 'due_date command' do
+      let(:content) { '/due tomorrow' }
+      let(:issuable) { issue }
+      let(:expected_date) { Date.tomorrow }
+    end
+
     it_behaves_like 'empty command' do
       let(:content) { '/due_date foo bar' }
       let(:issuable) { issue }