Merge branch '24915_merge_slash_command' into 'master'

Support `/merge` slash command for MRs

Closes #24915

See merge request !7746

app/assets/javascripts/notes.js

@@ -3,6 +3,7 @@
 /* global GLForm */
 /* global Autosave */
 /* global ResolveService */
+/* global mrRefreshWidgetUrl */
 
 /*= require autosave */
 /*= require autosize */
@@ -244,6 +245,16 @@
     };
 
 
+    Notes.prototype.handleCreateChanges = function(note) {
+      if (typeof note === 'undefined') {
+        return;
+      }
+
+      if (note.commands_changes && note.commands_changes.indexOf('merge') !== -1) {
+        $.get(mrRefreshWidgetUrl);
+      }
+    };
+
     /*
     Render note in main comments area.
 
@@ -429,6 +440,7 @@
      */
 
     Notes.prototype.addNote = function(xhr, note, status) {
+      this.handleCreateChanges(note);
       return this.renderNote(note);
     };
 

app/controllers/projects/merge_requests_controller.rb

@@ -347,6 +347,16 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     end
   end
 
+  def merge_widget_refresh
+    if merge_request.in_progress_merge_commit_sha || merge_request.state == 'merged'
+      @status = :success
+    elsif merge_request.merge_when_build_succeeds
+      @status = :merge_when_build_succeeds
+    end
+
+    render 'merge'
+  end
+
   def branch_from
     # This is always source
     @source_project = @merge_request.nil? ? @project : @merge_request.source_project

app/controllers/projects/notes_controller.rb

@@ -23,7 +23,8 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def create
-    @note = Notes::CreateService.new(project, current_user, note_params).execute
+    create_params = note_params.merge(merge_request_diff_head_sha: params[:merge_request_diff_head_sha])
+    @note = Notes::CreateService.new(project, current_user, create_params).execute
 
     if @note.is_a?(Note)
       Banzai::NoteRenderer.render([@note], @project, current_user)

app/helpers/merge_requests_helper.rb

@@ -19,6 +19,14 @@ module MergeRequestsHelper
     }
   end
 
+  def mr_widget_refresh_url(mr)
+    if mr && mr.source_project
+      merge_widget_refresh_namespace_project_merge_request_url(mr.source_project.namespace, mr.source_project, mr)
+    else
+      ''
+    end
+  end
+
   def mr_css_classes(mr)
     classes = "merge-request"
     classes << " closed" if mr.closed?

app/models/merge_request.rb

@@ -898,10 +898,22 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def has_commits?
-    commits_count > 0
+    merge_request_diff && commits_count > 0
   end
 
   def has_no_commits?
     !has_commits?
   end
+
+  def mergeable_with_slash_command?(current_user, autocomplete_precheck: false, last_diff_sha: nil)
+    return false unless can_be_merged_by?(current_user)
+
+    return true if autocomplete_precheck
+
+    return false unless mergeable?(skip_ci_check: true)
+    return false if head_pipeline && !(head_pipeline.success? || head_pipeline.active?)
+    return false if last_diff_sha != diff_head_sha
+
+    true
+  end
 end

app/services/merge_requests/update_service.rb

@@ -7,6 +7,8 @@ module MergeRequests
       params.except!(:target_project_id)
       params.except!(:source_branch)
 
+      merge_from_slash_command(merge_request) if params[:merge]
+
       if merge_request.closed_without_fork?
         params.except!(:target_branch, :force_remove_source_branch)
       end
@@ -69,6 +71,19 @@ module MergeRequests
       end
     end
 
+    def merge_from_slash_command(merge_request)
+      last_diff_sha = params.delete(:merge)
+      return unless merge_request.mergeable_with_slash_command?(current_user, last_diff_sha: last_diff_sha)
+
+      merge_request.update(merge_error: nil)
+
+      if merge_request.head_pipeline && merge_request.head_pipeline.active?
+        MergeRequests::MergeWhenPipelineSucceedsService.new(project, current_user).execute(merge_request)
+      else
+        MergeWorker.perform_async(merge_request.id, current_user.id, {})
+      end
+    end
+
     def reopen_service
       MergeRequests::ReopenService
     end

app/services/notes/create_service.rb

 module Notes
   class CreateService < BaseService
     def execute
+      merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha)
+
       note = project.notes.new(params)
       note.author = current_user
       note.system = false
@@ -19,7 +21,8 @@ module Notes
       slash_commands_service = SlashCommandsService.new(project, current_user)
 
       if slash_commands_service.supported?(note)
-        content, command_params = slash_commands_service.extract_commands(note)
+        options = { merge_request_diff_head_sha: merge_request_diff_head_sha }
+        content, command_params = slash_commands_service.extract_commands(note, options)
 
         only_commands = content.empty?
 

app/services/notes/slash_commands_service.rb

@@ -19,10 +19,10 @@ module Notes
       self.class.supported?(note, current_user)
     end
 
-    def extract_commands(note)
+    def extract_commands(note, options = {})
       return [note.note, {}] unless supported?(note)
 
-      SlashCommands::InterpretService.new(project, current_user).
+      SlashCommands::InterpretService.new(project, current_user, options).
         execute(note.note, note.noteable)
     end
 

app/services/slash_commands/interpret_service.rb

@@ -2,7 +2,7 @@ module SlashCommands
   class InterpretService < BaseService
     include Gitlab::SlashCommands::Dsl
 
-    attr_reader :issuable
+    attr_reader :issuable, :options
 
     # Takes a text and interprets the commands that are extracted from it.
     # Returns the content without commands, and hash of changes to be applied to a record.
@@ -13,7 +13,8 @@ module SlashCommands
       opts = {
         issuable:     issuable,
         current_user: current_user,
-        project:      project
+        project:      project,
+        params:       params
       }
 
       content, commands = extractor.extract_commands(content, opts)
@@ -58,6 +59,17 @@ module SlashCommands
       @updates[:state_event] = 'reopen'
     end
 
+    desc 'Merge (when build succeeds)'
+    condition do
+      last_diff_sha = params && params[:merge_request_diff_head_sha]
+      issuable.is_a?(MergeRequest) &&
+        issuable.persisted? &&
+        issuable.mergeable_with_slash_command?(current_user, autocomplete_precheck: !last_diff_sha, last_diff_sha: last_diff_sha)
+    end
+    command :merge do
+      @updates[:merge] = params[:merge_request_diff_head_sha]
+    end
+
     desc 'Change title'
     params '<New title>'
     condition do

app/views/projects/merge_requests/_show.html.haml

@@ -112,3 +112,6 @@
   merge_request = new MergeRequest({
     action: "#{controller.action_name}"
   });
+
+  var mrRefreshWidgetUrl = "#{mr_widget_refresh_url(@merge_request)}";
+

app/views/projects/notes/_form.html.haml

@@ -3,6 +3,7 @@
 = form_for [@project.namespace.becomes(Namespace), @project, @note], remote: true, html: { :'data-type' => 'json', multipart: true, id: nil, class: "new-note js-new-note-form js-quick-submit common-note-form", "data-noteable-iid" => @note.noteable.try(:iid), }, authenticity_token: true do |f|
   = hidden_field_tag :view, diff_view
   = hidden_field_tag :line_type
+  = hidden_field_tag :merge_request_diff_head_sha, @note.noteable.try(:diff_head_sha)
   = note_target_fields(@note)
   = f.hidden_field :commit_id
   = f.hidden_field :line_code

changelogs/unreleased/24915_merge_slash_command.yml

+---
+title: Support slash comand `/merge` for merging merge requests.
+merge_request: 7746
+author: Jarka Kadlecova

config/routes/project.rb

@@ -94,6 +94,7 @@ constraints(ProjectUrlConstrainer.new) do
           get :pipelines
           get :merge_check
           post :merge
+          get :merge_widget_refresh
           post :cancel_merge_when_build_succeeds
           get :ci_status
           get :ci_environments_status

doc/user/project/slash_commands.md

@@ -14,6 +14,7 @@ do.
 |:---------------------------|:-------------|
 | `/close`                   | Close the issue or merge request |
 | `/reopen`                  | Reopen the issue or merge request |
+| `/merge`                   | Merge (when build succeeds) |
 | `/title <New title>`       | Change title |
 | `/assign @username`        | Assign |
 | `/unassign`                | Remove assignee |

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -1048,4 +1048,72 @@ describe Projects::MergeRequestsController do
       end
     end
   end
+
+  describe 'GET merge_widget_refresh' do
+    let(:params) do
+      {
+        namespace_id: project.namespace.path,
+        project_id: project.path,
+        id: merge_request.iid,
+        format: :raw
+      }
+    end
+
+    before do
+      project.team << [user, :developer]
+      xhr :get, :merge_widget_refresh, params
+    end
+
+    context 'when merge in progress' do
+      let(:merge_request) { create(:merge_request, source_project: project, in_progress_merge_commit_sha: 'sha') }
+
+      it 'returns an OK response' do
+        expect(response).to have_http_status(:ok)
+      end
+
+      it 'sets status to :success' do
+        expect(assigns(:status)).to eq(:success)
+        expect(response).to render_template('merge')
+      end
+    end
+
+    context 'when merge request was merged already' do
+      let(:merge_request) { create(:merge_request, source_project: project, state: :merged) }
+
+      it 'returns an OK response' do
+        expect(response).to have_http_status(:ok)
+      end
+
+      it 'sets status to :success' do
+        expect(assigns(:status)).to eq(:success)
+        expect(response).to render_template('merge')
+      end
+    end
+
+    context 'when waiting for build' do
+      let(:merge_request) { create(:merge_request, source_project: project, merge_when_build_succeeds: true, merge_user: user) }
+
+      it 'returns an OK response' do
+        expect(response).to have_http_status(:ok)
+      end
+
+      it 'sets status to :merge_when_build_succeeds' do
+        expect(assigns(:status)).to eq(:merge_when_build_succeeds)
+        expect(response).to render_template('merge')
+      end
+    end
+
+    context 'when no special status for MR' do
+      let(:merge_request) { create(:merge_request, source_project: project) }
+
+      it 'returns an OK response' do
+        expect(response).to have_http_status(:ok)
+      end
+
+      it 'sets status to nil' do
+        expect(assigns(:status)).to be_nil
+        expect(response).to render_template('merge')
+      end
+    end
+  end
 end

spec/controllers/projects/notes_controller_spec.rb

@@ -14,6 +14,54 @@ describe Projects::NotesController do
     }
   end
 
+  describe 'POST create' do
+    let(:merge_request) { create(:merge_request) }
+    let(:request_params) do
+      {
+        note: { note: 'some note', noteable_id: merge_request.id, noteable_type: 'MergeRequest' },
+        namespace_id: project.namespace,
+        project_id: project,
+        merge_request_diff_head_sha: 'sha'
+      }
+    end
+
+    before do
+      sign_in(user)
+      project.team << [user, :developer]
+    end
+
+    it "returns status 302 for html" do
+      post :create, request_params
+
+      expect(response).to have_http_status(302)
+    end
+
+    it "returns status 200 for json" do
+      post :create, request_params.merge(format: :json)
+
+      expect(response).to have_http_status(200)
+    end
+
+    context 'when merge_request_diff_head_sha present' do
+      before do
+        service_params = {
+          note: 'some note',
+          noteable_id: merge_request.id.to_s,
+          noteable_type: 'MergeRequest',
+          merge_request_diff_head_sha: 'sha'
+        }
+
+        expect(Notes::CreateService).to receive(:new).with(project, user, service_params).and_return(double(execute: true))
+      end
+
+      it "returns status 302 for html" do
+        post :create, request_params
+
+        expect(response).to have_http_status(302)
+      end
+    end
+  end
+
   describe 'POST toggle_award_emoji' do
     before do
       sign_in(user)

spec/features/merge_requests/user_uses_slash_commands_spec.rb

@@ -68,6 +68,51 @@ feature 'Merge Requests > User uses slash commands', feature: true, js: true do
       end
     end
 
+    describe 'merging the MR from the note' do
+      context 'when the current user can merge the MR' do
+        it 'merges the MR' do
+          write_note("/merge")
+
+          expect(page).to have_content 'Commands applied'
+
+          expect(merge_request.reload).to be_merged
+        end
+      end
+
+      context 'when the head diff changes in the meanwhile' do
+        before do
+          merge_request.source_branch = 'another_branch'
+          merge_request.save
+        end
+
+        it 'does not merge the MR' do
+          write_note("/merge")
+
+          expect(page).not_to have_content 'Your commands have been executed!'
+
+          expect(merge_request.reload).not_to be_merged
+        end
+      end
+
+      context 'when the current user cannot merge the MR' do
+        let(:guest) { create(:user) }
+        before do
+          project.team << [guest, :guest]
+          logout
+          login_with(guest)
+          visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+        end
+
+        it 'does not merge the MR' do
+          write_note("/merge")
+
+          expect(page).not_to have_content 'Your commands have been executed!'
+
+          expect(merge_request.reload).not_to be_merged
+        end
+      end
+    end
+
     describe 'adding a due date from note' do
       it 'does not recognize the command nor create a note' do
         write_note('/due 2016-08-28')

spec/helpers/merge_requests_helper_spec.rb

@@ -62,4 +62,19 @@ describe MergeRequestsHelper do
       it { is_expected.to eq([source_title, target_title]) }
     end
   end
+
+  describe 'mr_widget_refresh_url' do
+    let(:merge_request) { create(:merge_request, source_project: project) }
+    let(:project)       { create(:project) }
+
+    it 'returns correct url for MR' do
+      expected_url = "#{project.path_with_namespace}/merge_requests/#{merge_request.iid}/merge_widget_refresh"
+
+      expect(mr_widget_refresh_url(merge_request)).to end_with(expected_url)
+    end
+
+    it 'returns empty string for nil' do
+      expect(mr_widget_refresh_url(nil)).to end_with('')
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -1514,6 +1514,108 @@ describe MergeRequest, models: true do
     end
   end
 
+  describe '#mergeable_with_slash_command?' do
+    def create_pipeline(status)
+      create(:ci_pipeline_with_one_job,
+        project: project,
+        ref:     merge_request.source_branch,
+        sha:     merge_request.diff_head_sha,
+        status:  status)
+    end
+
+    let(:project)       { create(:project, :public, only_allow_merge_if_build_succeeds: true) }
+    let(:developer)     { create(:user) }
+    let(:user)          { create(:user) }
+    let(:merge_request) { create(:merge_request, source_project: project) }
+    let(:mr_sha)        { merge_request.diff_head_sha }
+
+    before do
+      project.team << [developer, :developer]
+    end
+
+    context 'when autocomplete_precheck is set to true' do
+      it 'is mergeable by developer' do
+        expect(merge_request.mergeable_with_slash_command?(developer, autocomplete_precheck: true)).to be_truthy
+      end
+
+      it 'is not mergeable by normal user' do
+        expect(merge_request.mergeable_with_slash_command?(user, autocomplete_precheck: true)).to be_falsey
+      end
+    end
+
+    context 'when autocomplete_precheck is set to false' do
+      it 'is mergeable by developer' do
+        expect(merge_request.mergeable_with_slash_command?(developer, last_diff_sha: mr_sha)).to be_truthy
+      end
+
+      it 'is not mergeable by normal user' do
+        expect(merge_request.mergeable_with_slash_command?(user, last_diff_sha: mr_sha)).to be_falsey
+      end
+
+      context 'closed MR'  do
+        before do
+          merge_request.update_attribute(:state, :closed)
+        end
+
+        it 'is not mergeable' do
+          expect(merge_request.mergeable_with_slash_command?(developer, last_diff_sha: mr_sha)).to be_falsey
+        end
+      end
+
+      context 'MR with WIP'  do
+        before do
+          merge_request.update_attribute(:title, 'WIP: some MR')
+        end
+
+        it 'is not mergeable' do
+          expect(merge_request.mergeable_with_slash_command?(developer, last_diff_sha: mr_sha)).to be_falsey
+        end
+      end
+
+      context 'sha differs from the MR diff_head_sha'  do
+        it 'is not mergeable' do
+          expect(merge_request.mergeable_with_slash_command?(developer, last_diff_sha: 'some other sha')).to be_falsey
+        end
+      end
+
+      context 'sha is not provided'  do
+        it 'is not mergeable' do
+          expect(merge_request.mergeable_with_slash_command?(developer)).to be_falsey
+        end
+      end
+
+      context 'with pipeline ok'  do
+        before do
+          create_pipeline(:success)
+        end
+
+        it 'is mergeable' do
+          expect(merge_request.mergeable_with_slash_command?(developer, last_diff_sha: mr_sha)).to be_truthy
+        end
+      end
+
+      context 'with failing pipeline'  do
+        before do
+          create_pipeline(:failed)
+        end
+
+        it 'is not mergeable' do
+          expect(merge_request.mergeable_with_slash_command?(developer, last_diff_sha: mr_sha)).to be_falsey
+        end
+      end
+
+      context 'with running pipeline'  do
+        before do
+          create_pipeline(:running)
+        end
+
+        it 'is mergeable' do
+          expect(merge_request.mergeable_with_slash_command?(developer, last_diff_sha: mr_sha)).to be_truthy
+        end
+      end
+    end
+  end
+
   describe '#has_commits?' do
     before do
       allow(subject.merge_request_diff).to receive(:commits_count).

spec/services/merge_requests/update_service_spec.rb

@@ -121,6 +121,99 @@ describe MergeRequests::UpdateService, services: true do
       end
     end
 
+    context 'merge' do
+      let(:opts) do
+        {
+          merge: merge_request.diff_head_sha
+        }
+      end
+
+      let(:service) { MergeRequests::UpdateService.new(project, user, opts) }
+
+      context 'without pipeline' do
+        before do
+          merge_request.merge_error = 'Error'
+
+          perform_enqueued_jobs do
+            service.execute(merge_request)
+            @merge_request = MergeRequest.find(merge_request.id)
+          end
+        end
+
+        it { expect(@merge_request).to be_valid }
+        it { expect(@merge_request.state).to eq('merged') }
+        it { expect(@merge_request.merge_error).to be_nil }
+      end
+
+      context 'with finished pipeline' do
+        before do
+          create(:ci_pipeline_with_one_job,
+            project: project,
+            ref:     merge_request.source_branch,
+            sha:     merge_request.diff_head_sha,
+            status:  :success)
+
+          perform_enqueued_jobs do
+            @merge_request = service.execute(merge_request)
+            @merge_request = MergeRequest.find(merge_request.id)
+          end
+        end
+
+        it { expect(@merge_request).to be_valid }
+        it { expect(@merge_request.state).to eq('merged') }
+      end
+
+      context 'with active pipeline' do
+        before do
+          service_mock = double
+          create(:ci_pipeline_with_one_job,
+            project: project,
+            ref:     merge_request.source_branch,
+            sha:     merge_request.diff_head_sha)
+
+          expect(MergeRequests::MergeWhenPipelineSucceedsService).to receive(:new).with(project, user).
+            and_return(service_mock)
+          expect(service_mock).to receive(:execute).with(merge_request)
+        end
+
+        it { service.execute(merge_request) }
+      end
+
+      context 'with a non-authorised user' do
+        let(:visitor) { create(:user) }
+        let(:service) { MergeRequests::UpdateService.new(project, visitor, opts) }
+
+        before do
+          merge_request.update_attribute(:merge_error, 'Error')
+
+          perform_enqueued_jobs do
+            @merge_request = service.execute(merge_request)
+            @merge_request = MergeRequest.find(merge_request.id)
+          end
+        end
+
+        it { expect(@merge_request.state).to eq('opened') }
+        it { expect(@merge_request.merge_error).not_to be_nil }
+      end
+
+      context 'MR can not be merged when note sha != MR sha' do
+        let(:opts) do
+          {
+            merge: 'other_commit'
+          }
+        end
+
+        before do
+          perform_enqueued_jobs do
+            @merge_request = service.execute(merge_request)
+            @merge_request = MergeRequest.find(merge_request.id)
+          end
+        end
+
+        it { expect(@merge_request.state).to eq('opened') }
+      end
+    end
+
     context 'todos' do
       let!(:pending_todo) { create(:todo, :assigned, user: user, project: project, target: merge_request, author: user2) }
 

spec/services/notes/create_service_spec.rb

@@ -63,6 +63,17 @@ describe Notes::CreateService, services: true do
           expect(note.note).to eq "HELLO\nWORLD"
         end
       end
+
+      describe '/merge with sha option' do
+        let(:note_text) { %(HELLO\n/merge\nWORLD) }
+        let(:params) { opts.merge(note: note_text, merge_request_diff_head_sha: 'sha') }
+
+        it 'saves the note and exectues merge command' do
+          note = described_class.new(project, user, params).execute
+
+          expect(note.note).to eq "HELLO\nWORLD"
+        end
+      end
     end
   end
 

spec/services/slash_commands/interpret_service_spec.rb

 require 'spec_helper'
 
 describe SlashCommands::InterpretService, services: true do
-  let(:project) { create(:empty_project, :public) }
+  let(:project) { create(:project, :public) }
   let(:developer) { create(:user) }
   let(:issue) { create(:issue, project: project) }
   let(:milestone) { create(:milestone, project: project, title: '9.10') }
   let(:inprogress) { create(:label, project: project, title: 'In Progress') }
   let(:bug) { create(:label, project: project, title: 'Bug') }
+  let(:note) { build(:note, commit_id: merge_request.diff_head_sha) }
 
   before do
     project.team << [developer, :developer]
@@ -218,6 +219,14 @@ describe SlashCommands::InterpretService, services: true do
       end
     end
 
+    shared_examples 'merge command' do
+      it 'runs merge command if content contains /merge' do
+        _, updates = service.execute(content, issuable)
+
+        expect(updates).to eq(merge: merge_request.diff_head_sha)
+      end
+    end
+
     it_behaves_like 'reopen command' do
       let(:content) { '/reopen' }
       let(:issuable) { issue }
@@ -238,6 +247,64 @@ describe SlashCommands::InterpretService, services: true do
       let(:issuable) { merge_request }
     end
 
+    context 'merge command' do
+      let(:service) { described_class.new(project, developer, { merge_request_diff_head_sha: merge_request.diff_head_sha }) }
+
+      it_behaves_like 'merge command' do
+        let(:content) { '/merge' }
+        let(:issuable) { merge_request }
+      end
+
+      context 'can not be merged when logged user does not have permissions' do
+        let(:service) { described_class.new(project, create(:user)) }
+
+        it_behaves_like 'empty command' do
+          let(:content) { "/merge" }
+          let(:issuable) { merge_request }
+        end
+      end
+
+      context 'can not be merged when sha does not match' do
+        let(:service) { described_class.new(project, developer, { merge_request_diff_head_sha: 'othersha' }) }
+
+        it_behaves_like 'empty command' do
+          let(:content) { "/merge" }
+          let(:issuable) { merge_request }
+        end
+      end
+
+      context 'when sha is missing' do
+        let(:service) { described_class.new(project, developer, {}) }
+
+        it 'precheck passes and returns merge command' do
+          _, updates = service.execute('/merge', merge_request)
+
+          expect(updates).to eq(merge: nil)
+        end
+      end
+
+      context 'issue can not be merged' do
+        it_behaves_like 'empty command' do
+          let(:content) { "/merge" }
+          let(:issuable) { issue }
+        end
+      end
+
+      context 'non persisted merge request  cant be merged' do
+        it_behaves_like 'empty command' do
+          let(:content) { "/merge" }
+          let(:issuable) { build(:merge_request) }
+        end
+      end
+
+      context 'not persisted merge request can not be merged' do
+        it_behaves_like 'empty command' do
+          let(:content) { "/merge" }
+          let(:issuable) { build(:merge_request, source_project: project) }
+        end
+      end
+    end
+
     it_behaves_like 'title command' do
       let(:content) { '/title A brand new title' }
       let(:issuable) { issue }