New Notes::SlashCommandsService service

Check for update_issuable permission in Notes::SlashCommandsService
Signed-off-by: Rémy Coutable <remy@rymai.me>

app/services/issuable_base_service.rb

@@ -81,21 +81,21 @@ class IssuableBaseService < BaseService
   end
 
   def process_label_ids(attributes, base_label_ids: [], merge_all: false)
-    label_ids = attributes.delete(:label_ids) { [] }
-    add_label_ids = attributes.delete(:add_label_ids) { [] }
-    remove_label_ids = attributes.delete(:remove_label_ids) { [] }
+    label_ids = attributes.delete(:label_ids)
+    add_label_ids = attributes.delete(:add_label_ids)
+    remove_label_ids = attributes.delete(:remove_label_ids)
 
     new_label_ids = base_label_ids
-    new_label_ids |= label_ids if merge_all || (add_label_ids.empty? && remove_label_ids.empty?)
-    new_label_ids |= add_label_ids
-    new_label_ids -= remove_label_ids
+    new_label_ids = label_ids if label_ids && (merge_all || (add_label_ids.empty? && remove_label_ids.empty?))
+    new_label_ids |= add_label_ids if add_label_ids
+    new_label_ids -= remove_label_ids if remove_label_ids
 
     new_label_ids
   end
 
-  def merge_slash_commands_into_params!
+  def merge_slash_commands_into_params!(issuable)
     command_params = SlashCommands::InterpretService.new(project, current_user).
-      execute(params[:description])
+      execute(params[:description], issuable)
 
     params.merge!(command_params)
   end
@@ -115,7 +115,7 @@ class IssuableBaseService < BaseService
   end
 
   def create(issuable)
-    merge_slash_commands_into_params!
+    merge_slash_commands_into_params!(issuable)
     filter_params
 
     if params.present? && create_issuable(issuable, params)

app/services/notes/create_service.rb

@@ -14,7 +14,7 @@ module Notes
       # We execute commands (extracted from `params[:note]`) on the noteable
       # **before** we save the note because if the note consists of commands
       # only, there is no need be create a note!
-      commands_executed = execute_slash_commands!(note)
+      commands_executed = SlashCommandsService.new(project, current_user).execute(note)
 
       if note.save
         # Finish the harder work in the background
@@ -22,50 +22,13 @@ module Notes
         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 commands_executed && note.note.blank?
         note.errors.add(:commands_only, 'Your commands are being executed.')
       end
 
       note
     end
-
-    private
-
-    def execute_slash_commands!(note)
-      noteable_update_service = noteable_update_service(note.noteable_type)
-      return unless noteable_update_service
-
-      command_params = SlashCommands::InterpretService.new(project, current_user).
-        execute(note.note)
-
-      commands = execute_or_filter_commands(command_params, note)
-
-      if commands.any?
-        noteable_update_service.new(project, current_user, commands).execute(note.noteable)
-      end
-    end
-
-    def execute_or_filter_commands(commands, note)
-      final_commands = commands.reduce({}) do |memo, (command_key, command_value)|
-        if command_key != :due_date || note.noteable.respond_to?(:due_date)
-          memo[command_key] = command_value
-        end
-
-        memo
-      end
-
-      final_commands
-    end
-
-    def noteable_update_service(noteable_type)
-      case noteable_type
-      when 'Issue'
-        Issues::UpdateService
-      when 'MergeRequest'
-        MergeRequests::UpdateService
-      else
-        nil
-      end
-    end
   end
 end

app/services/notes/slash_commands_service.rb

+module Notes
+  class SlashCommandsService < BaseService
+
+    UPDATE_SERVICES = {
+      'Issue' => Issues::UpdateService,
+      'MergeRequest' => MergeRequests::UpdateService
+    }
+
+    def execute(note)
+      noteable_update_service = UPDATE_SERVICES[note.noteable_type]
+      return false unless noteable_update_service
+      return false unless can?(current_user, :"update_#{note.noteable_type.underscore}", note.noteable)
+
+      commands = SlashCommands::InterpretService.new(project, current_user).
+        execute(note.note, note.noteable)
+
+      if commands.any?
+        noteable_update_service.new(project, current_user, commands).execute(note.noteable)
+      end
+    end
+  end
+end

app/services/slash_commands/interpret_service.rb

@@ -2,9 +2,12 @@ module SlashCommands
   class InterpretService < BaseService
     include Gitlab::SlashCommands::Dsl
 
+    attr_reader :noteable
+
     # 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)
+    def execute(content, noteable)
+      @noteable = noteable
       @updates = {}
 
       commands = extractor.extract_commands!(content)
@@ -105,6 +108,8 @@ module SlashCommands
     desc 'Set a due date'
     params '<YYYY-MM-DD> | <N days>'
     command :due_date do |due_date_param|
+      return unless noteable.respond_to?(:due_date)
+
       due_date = begin
         Time.now + ChronicDuration.parse(due_date_param)
       rescue ChronicDuration::DurationParseError
@@ -116,6 +121,8 @@ module SlashCommands
 
     desc 'Remove due date'
     command :clear_due_date do
+      return unless noteable.respond_to?(:due_date)
+
       @updates[:due_date] = nil
     end
 

spec/features/merge_requests/user_uses_slash_commands_spec.rb

@@ -29,31 +29,4 @@ feature 'Merge Requests > User uses slash commands', feature: true, js: true do
       expect(page).not_to have_content '/due_date 2016-08-28'
     end
   end
-
-  # Postponed because of high complexity
-  xdescribe 'merging from note' do
-    before do
-      project.team << [user, :master]
-      login_with(user)
-      visit namespace_project_merge_request_path(project.namespace, project, merge_request)
-    end
-
-    it 'creates a note without the commands and interpret the commands accordingly' do
-      page.within('.js-main-target-form') do
-        fill_in 'note[note]', with: "Let's merge this!\n/merge\n/milestone %ASAP"
-        click_button 'Comment'
-      end
-
-      expect(page).to have_content("Let's merge this!")
-      expect(page).not_to have_content('/merge')
-      expect(page).not_to have_content('/milestone %ASAP')
-
-      merge_request.reload
-      note = merge_request.notes.user.first
-
-      expect(note.note).to eq "Let's merge this!\r\n"
-      expect(merge_request).to be_merged
-      expect(merge_request.milestone).to eq milestone
-    end
-  end
 end

spec/services/notes/create_service_spec.rb

@@ -9,25 +9,30 @@ describe Notes::CreateService, services: true do
   end
 
   describe '#execute' do
-    context "valid params" do
     before do
       project.team << [user, :master]
+    end
+
+    context "valid params" do
+      before do
         @note = Notes::CreateService.new(project, user, opts).execute
       end
 
       it { expect(@note).to be_valid }
       it { expect(@note.note).to eq(opts[:note]) }
-
-      it_behaves_like 'note on noteable that supports slash commands' do
-        let(:noteable) { create(:issue, project: project) }
     end
 
-      it_behaves_like 'note on noteable that supports slash commands' do
-        let(:noteable) { create(:merge_request, source_project: project) }
-      end
+    describe 'note with commands' do
+      describe '/close, /label, /assign & /milestone' do
+        let(:note_text) { %(HELLO\n/close\n/assign @#{user.username}\nWORLD) }
+
+        it 'saves the note and does not alter the note text' do
+          expect_any_instance_of(Issues::UpdateService).to receive(:execute).and_call_original
 
-      it_behaves_like 'note on noteable that does not support slash commands' do
-        let(:noteable) { create(:commit, project: project) }
+          note = described_class.new(project, user, opts.merge(note: note_text)).execute
+
+          expect(note.note).to eq "HELLO\nWORLD"
+        end
       end
     end
   end

spec/services/notes/slash_commands_service_spec.rb

+require 'spec_helper'
+
+describe Notes::SlashCommandsService, services: true do
+  shared_context 'note on noteable' do
+    let(:project) { create(:empty_project) }
+    let(:master) { create(:user).tap { |u| project.team << [u, :master] } }
+    let(:assignee) { create(:user) }
+  end
+
+  shared_examples 'note on noteable that does not support slash commands' do
+    include_context 'note on noteable'
+
+    before do
+      note.note = note_text
+      described_class.new(project, master).execute(note)
+    end
+
+    describe 'note with only command' do
+      describe '/close, /label, /assign & /milestone' 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
+        end
+      end
+    end
+
+    describe 'note with command & text' do
+      describe '/close, /label, /assign & /milestone' 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
+        end
+      end
+    end
+  end
+
+  shared_examples 'note on noteable that supports slash commands' do
+    include_context 'note on noteable'
+
+    before do
+      note.note = note_text
+    end
+
+    let!(:milestone) { create(:milestone, project: project) }
+    let!(:labels) { create_pair(:label, project: project) }
+
+    describe 'note with only command' do
+      describe '/close, /label, /assign & /milestone' do
+        let(:note_text) do
+          %(/close\n/label ~#{labels.first.name} ~#{labels.last.name}\n/assign @#{assignee.username}\n/milestone %"#{milestone.name}")
+        end
+
+        it 'closes noteable, sets labels, assigns, and sets milestone to noteable, and leave no note' do
+          described_class.new(project, master).execute(note)
+
+          expect(note.note).to eq ''
+          expect(note.noteable).to be_closed
+          expect(note.noteable.labels).to match_array(labels)
+          expect(note.noteable.assignee).to eq(assignee)
+          expect(note.noteable.milestone).to eq(milestone)
+        end
+      end
+
+      describe '/open' do
+        before do
+          note.noteable.close!
+          expect(note.noteable).to be_closed
+        end
+        let(:note_text) { '/open' }
+
+        it 'opens the noteable, and leave no note' do
+          described_class.new(project, master).execute(note)
+
+          expect(note.note).to eq ''
+          expect(note.noteable).to be_open
+        end
+      end
+    end
+
+    describe 'note with command & text' do
+      describe '/close, /label, /assign & /milestone' do
+        let(:note_text) do
+          %(HELLO\n/close\n/label ~#{labels.first.name} ~#{labels.last.name}\n/assign @#{assignee.username}\n/milestone %"#{milestone.name}"\nWORLD)
+        end
+
+        it 'closes noteable, sets labels, assigns, and sets milestone to noteable' do
+          described_class.new(project, master).execute(note)
+
+          expect(note.note).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)
+          expect(note.noteable.milestone).to eq(milestone)
+        end
+      end
+
+      describe '/open' do
+        before do
+          note.noteable.close
+          expect(note.noteable).to be_closed
+        end
+        let(:note_text) { "HELLO\n/open\nWORLD" }
+
+        it 'opens the noteable' do
+          described_class.new(project, master).execute(note)
+
+          expect(note.note).to eq "HELLO\nWORLD"
+          expect(note.noteable).to be_open
+        end
+      end
+    end
+  end
+
+  describe '#execute' do
+    it_behaves_like 'note on noteable that supports slash commands' do
+      let(:note) { build(:note_on_issue, project: project) }
+    end
+
+    it_behaves_like 'note on noteable that supports slash commands' do
+      let(:note) { build(:note_on_merge_request, project: project) }
+    end
+
+    it_behaves_like 'note on noteable that does not support slash commands' do
+      let(:note) { build(:note_on_commit, project: project) }
+    end
+  end
+end

spec/support/issuable_slash_commands_shared_examples.rb

@@ -118,7 +118,7 @@ shared_examples 'issuable record that supports slash commands in its description
           end
 
           expect(page).not_to have_content '/close'
-          expect(page).to have_content 'Your commands are being executed.'
+          expect(page).not_to have_content 'Your commands are being executed.'
 
           expect(issuable).to be_open
         end
@@ -159,7 +159,7 @@ shared_examples 'issuable record that supports slash commands in its description
           end
 
           expect(page).not_to have_content '/reopen'
-          expect(page).to have_content 'Your commands are being executed.'
+          expect(page).not_to have_content 'Your commands are being executed.'
 
           expect(issuable).to be_closed
         end

spec/support/note_create_service_slash_commands_shared_examples.rb

-# Specifications for behavior common to all note objects with executable attributes.
-# It expects a `noteable` object for which the note is posted.
-
-shared_context 'note on noteable' do
-  let!(:project) { create(:project) }
-  let(:user) { create(:user).tap { |u| project.team << [u, :master] } }
-  let(:assignee) { create(:user) }
-  let(:base_params) { { noteable: noteable } }
-  let(:params) { base_params.merge(example_params) }
-  let(:note) { described_class.new(project, user, params).execute }
-end
-
-shared_examples 'note on noteable that does not support slash commands' do
-  include_context 'note on noteable'
-
-  let(:params) { { commit_id: noteable.id, noteable_type: 'Commit' }.merge(example_params) }
-
-  describe 'note with only command' do
-    describe '/close, /label, /assign & /milestone' do
-      let(:note_text) { %(/close\n/assign @#{assignee.username}") }
-      let(:example_params) { { note: note_text } }
-
-      it 'saves the note and does not alter the note text' do
-        expect(note).to be_persisted
-        expect(note.note).to eq note_text
-      end
-    end
-  end
-
-  describe 'note with command & text' do
-    describe '/close, /label, /assign & /milestone' do
-      let(:note_text) { %(HELLO\n/close\n/assign @#{assignee.username}\nWORLD) }
-      let(:example_params) { { note: note_text } }
-
-      it 'saves the note and does not alter the note text' do
-        expect(note).to be_persisted
-        expect(note.note).to eq note_text
-      end
-    end
-  end
-end
-
-shared_examples 'note on noteable that supports slash commands' do
-  include_context 'note on noteable'
-
-  let!(:milestone) { create(:milestone, project: project) }
-  let!(:labels) { create_pair(:label, project: project) }
-
-  describe 'note with only command' do
-    describe '/close, /label, /assign & /milestone' do
-      let(:example_params) do
-        {
-          note: %(/close\n/label ~#{labels.first.name} ~#{labels.last.name}\n/assign @#{assignee.username}\n/milestone %"#{milestone.name}")
-        }
-      end
-
-      it 'closes noteable, sets labels, assigns, and sets milestone to noteable, and leave no note' do
-        expect(note).not_to be_persisted
-        expect(note.note).to eq ''
-        expect(noteable).to be_closed
-        expect(noteable.labels).to match_array(labels)
-        expect(noteable.assignee).to eq(assignee)
-        expect(noteable.milestone).to eq(milestone)
-      end
-    end
-
-    describe '/open' do
-      let(:noteable) { create(:issue, project: project, state: :closed) }
-      let(:example_params) do
-        {
-          note: '/open'
-        }
-      end
-
-      it 'opens the noteable, and leave no note' do
-        expect(note).not_to be_persisted
-        expect(note.note).to eq ''
-        expect(noteable).to be_open
-      end
-    end
-  end
-
-  describe 'note with command & text' do
-    describe '/close, /label, /assign & /milestone' do
-      let(:example_params) do
-        {
-          note: %(HELLO\n/close\n/label ~#{labels.first.name} ~#{labels.last.name}\n/assign @#{assignee.username}\n/milestone %"#{milestone.name}"\nWORLD)
-        }
-      end
-
-      it 'closes noteable, sets labels, assigns, and sets milestone to noteable' do
-        expect(note).to be_persisted
-        expect(note.note).to eq "HELLO\nWORLD"
-        expect(noteable).to be_closed
-        expect(noteable.labels).to match_array(labels)
-        expect(noteable.assignee).to eq(assignee)
-        expect(noteable.milestone).to eq(milestone)
-      end
-    end
-
-    describe '/open' do
-      let(:noteable) { create(:issue, project: project, state: :closed) }
-      let(:example_params) do
-        {
-          note: "HELLO\n/open\nWORLD"
-        }
-      end
-
-      it 'opens the noteable' do
-        expect(note).to be_persisted
-        expect(note.note).to eq "HELLO\nWORLD"
-        expect(noteable).to be_open
-      end
-    end
-  end
-end