Refactor MergeWhenBuildSucceedsService and incorporate feedback

CHANGELOG

 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.3.0 (unreleased)
+- Merge when build succeeds (Zeger-Jan van de Weg)
 
 v 8.2.0
   - Fix grouping of contributors by email in graph.
@@ -28,10 +29,6 @@ v 8.2.0
   - Allow to define cache in `.gitlab-ci.yml`
   - Fix: 500 error returned if destroy request without HTTP referer (Kazuki Shimizu)
   - Remove deprecated CI events from project settings page
-  - Use issue editor as cross reference comment author when issue is edited with a new mention.
-  - Merge when build succeeds (Zeger-Jan van de Weg)
-
-v 8.1.1
   - [API] Add ability to fetch the commit ID of the last commit that actually touched a file
   - Fix omniauth documentation setting for omnibus configuration (Jon Cairns)
   - Add "New file" link to dropdown on project page

app/controllers/projects/merge_requests_controller.rb

@@ -151,14 +151,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def cancel_merge_when_build_succeeds
-    unless @merge_request.can_be_merged_by?(current_user) || @merge_request.author == current_user
-      return access_denied!
-    end
+    return access_denied! unless @merge_request.can_cancel_merge_when_build_succeeds?(current_user)
 
-    if @merge_request.merge_when_build_succeeds?
-      @merge_request.reset_merge_when_build_succeeds
-      SystemNoteService.cancel_merge_when_build_succeeds(merge_request, @project, @current_user)
-    end
+    MergeRequests::MergeWhenBuildSucceedsService.new(@project, current_user).cancel(@merge_request)
   end
 
   def merge
@@ -171,7 +166,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     @merge_request.update(merge_error: nil)
 
-    if params[:merge_when_build_succeeds] && @merge_request.ci_commit.active?
+    if params[:merge_when_build_succeeds] && @merge_request.ci_commit && @merge_request.ci_commit.active?
       MergeRequests::MergeWhenBuildSucceedsService.new(@project, current_user, merge_params)
                                                       .execute(@merge_request)
       @status = :merge_when_build_succeeds

app/models/merge_request.rb

@@ -254,8 +254,15 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  def mergeable_by_or_author(user)
-    self.can_be_merged_by?(user) || self.author == user
+  def can_cancel_merge_when_build_succeeds?(user)
+    can_be_merged_by?(user) || self.author == user
+  end
+
+  def can_remove_source_branch?
+    for_fork? &&
+    !project.protected_branch(source_branch) &&
+    !project.repository.root_ref(source_branch) &&
+    can?(current_user, :push_code, project)
   end
 
   def mr_and_commit_notes

app/services/merge_requests/merge_when_build_succeeds_service.rb

 module MergeRequests
   class MergeWhenBuildSucceedsService < MergeRequests::BaseService
+    # Marks the passed `merge_request` to be marked when the build succeeds or
+    # updates the params for the automatic merge
     def execute(merge_request)
       merge_request.merge_params.merge!(params)
 
@@ -14,10 +16,11 @@ module MergeRequests
       merge_request.save
 
       unless already_approved
-        SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user)
+        SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.ci_commit)
       end
     end
 
+    # Triggers the automatic merge of merge_request once the build succeeds
     def trigger(build)
       merge_requests = merge_request_from(build)
 
@@ -31,6 +34,18 @@ module MergeRequests
       end
     end
 
+    # Cancels the automatic merge
+    def cancel(merge_request)
+      if merge_request.merge_when_build_succeeds? && merge_request.open? && !merge_request.merged?
+        merge_request.reset_merge_when_build_succeeds
+        SystemNoteService.cancel_merge_when_build_succeeds(merge_request, @project, @current_user)
+
+        success
+      else
+        error("Can't cancel the automatic merge", 406)
+      end
+    end
+
     private
 
     def merge_request_from(build)

app/services/system_note_service.rb

@@ -131,8 +131,8 @@ class SystemNoteService
   end
 
   # Called when 'merge when build succeeds' is executed
-  def self.merge_when_build_succeeds(noteable, project, author)
-    body = "This merge request will be automatically merged when the build for #{noteable.ci_commit.short_sha} succeeds"
+  def self.merge_when_build_succeeds(noteable, project, author, ci_commit)
+    body = "Approved an automatic merge when the build for #{ci_commit.sha} succeeds"
 
     create_note(noteable: noteable, project: project, author: author, note: body)
   end

app/views/projects/merge_requests/widget/open/_accept.html.haml

@@ -13,7 +13,7 @@
       - else
         = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do
           Accept Merge Request
-    - if can_remove_branch?(@merge_request.source_project, @merge_request.source_branch) && !@merge_request.for_fork?
+    - if @merge_request.can_remove_source_branch?
       .accept-control.checkbox
         = label_tag :should_remove_source_branch, class: "remove_source_checkbox" do
           = check_box_tag :should_remove_source_branch

app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml

@@ -8,20 +8,17 @@
       The changes will be merged into
       %span.label-branch= @merge_request.target_branch
     The source branch will be removed.
-  - elsif can_remove_branch?(@merge_request.source_project, @merge_request.source_branch)
-    - remove_source_branch_button = true
     %p
       = succeed '.' do
         The changes will be merged into
         %span.label-branch= @merge_request.target_branch
-      The source branch won't be removed.
+      The source branch will not be removed.
 
-- if remove_source_branch_button || @merge_request.can_be_merged_by?(current_user)
   .clearfix.prepend-top-10
-    - if remove_source_branch_button
+    - if @merge_request.can_remove_source_branch? && !@merge_request.merge_params["should_remove_source_branch"].present?
       = link_to merge_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, merge_when_build_succeeds: true, should_remove_source_branch: true), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do
         = icon('times')
         Remove Source Branch When Merged
-    - if @merge_request.can_be_merged_by?(current_user) || @merge_request.author == current_user
+    - if @merge_request.merge_when_build_succeeds
       = link_to cancel_merge_when_build_succeeds_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request), remote: true, method: :post, class: "btn btn-grouped btn-warning btn-sm" do
         Cancel Automatic Merge

config/routes.rb

@@ -570,6 +570,7 @@ Gitlab::Application.routes.draw do
           member do
             get :diffs
             get :commits
+            get :merge_check
             post :merge
             post :cancel_merge_when_build_succeeds
             get :ci_status

db/schema.rb

@@ -421,7 +421,6 @@ ActiveRecord::Schema.define(version: 20151116144118) do
   end
 
   add_index "labels", ["project_id"], name: "index_labels_on_project_id", using: :btree
-  add_index "labels", ["title"], name: "index_labels_on_title", using: :btree
 
   create_table "lfs_objects", force: true do |t|
     t.string   "oid",        null: false
@@ -525,7 +524,6 @@ ActiveRecord::Schema.define(version: 20151116144118) do
   add_index "milestones", ["due_date"], name: "index_milestones_on_due_date", using: :btree
   add_index "milestones", ["project_id", "iid"], name: "index_milestones_on_project_id_and_iid", unique: true, using: :btree
   add_index "milestones", ["project_id"], name: "index_milestones_on_project_id", using: :btree
-  add_index "milestones", ["title"], name: "index_milestones_on_title", using: :btree
 
   create_table "namespaces", force: true do |t|
     t.string   "name",                        null: false

doc/api/merge_requests.md

@@ -296,7 +296,7 @@ Parameters:
 - `merge_request_id` (required)             - ID of MR
 - `merge_commit_message` (optional)         - Custom merge commit message
 - `should_remove_source_branch` (optional)  - if `true` removes the source branch
-- `merge_when_build_succeeds` (optional)    - if `true` the MR is merge when the build succeeds
+- `merged_when_build_succeeds` (optional)    - if `true` the MR is merge when the build succeeds
 
 ```json
 {
@@ -329,15 +329,13 @@ Parameters:
 
 ## Cancel Merge When Build Succeeds
 
-Cancels the merge when build succeeds and reset the merge parameters
-
-If successfull you'll get `200 OK`.
+If successful you'll get `200 OK`.
 
 If you don't have permissions to accept this merge request - you'll get a 401
 
 If the merge request is already merged or closed - you get 405 and error message 'Method Not Allowed'
 
-In case the merge request is not set to be merged when the build succeeds, you'll also get a 405 with the error message 'Method Not Allowed'
+In case the merge request is not set to be merged when the build succeeds, you'll also get a 406 error.
 ```
 PUT /projects/:id/merge_request/:merge_request_id/cancel_merge_when_build_succeeds
 ```

lib/api/merge_requests.rb

@@ -182,39 +182,35 @@ module API
       #   id (required)                         - The ID of a project
       #   merge_request_id (required)           - ID of MR
       #   merge_commit_message (optional)       - Custom merge commit message
-      #   merge_when_build_succeeds (optional)  - truethy when this MR should be merged when the build is succesfull
+      #   should_remove_source_branch           - When true, the source branch will be deleted if possible
+      #   merge_when_build_succeeds (optional)  - When true, this MR will be merged when the build succeeds
       # Example:
       #   PUT /projects/:id/merge_request/:merge_request_id/merge
       #
       put ":id/merge_request/:merge_request_id/merge" do
         merge_request = user_project.merge_requests.find(params[:merge_request_id])
 
-        allowed = ::Gitlab::GitAccess.new(current_user, user_project).
-          can_push_to_branch?(merge_request.target_branch)
-
         # Merge request can not be merged
         # because user dont have permissions to push into target branch
-        unauthorized! unless allowed
+        unauthorized! unless merge_request.can_be_merged_by?(current_user)
 
-        not_allowed! unless merge_request.open?
+        not_allowed! unless merge_request.open? && !merge_request.work_in_progress?
 
         merge_request.check_if_can_be_merged if merge_request.unchecked?
 
+        render_api_error!('Branch cannot be merged', 406) if merge_request.can_be_merged?
+
         merge_params = {
           commit_message: params[:merge_commit_message],
           should_remove_source_branch: params[:should_remove_source_branch]
         }
 
-        if  !merge_request.work_in_progress?
-          if parse_boolean(params[:merge_when_build_succeeds])
-            ::MergeRequest::MergeWhenBuildSucceedsService.new(merge_request.target_project, current_user, merge_params).
-              execute(merge_request)
-          else
-            ::MergeRequests::MergeService.new(merge_request.target_project, current_user, merge_params).
-              execute(merge_request, merge_params)
-          end
+        if parse_boolean(params[:merge_when_build_succeeds]) && merge_request.ci_commit && merge_request.ci_commit.active?
+          ::MergeRequest::MergeWhenBuildSucceedsService.new(merge_request.target_project, current_user, merge_params).
+            execute(merge_request)
         else
-          render_api_error!('Branch cannot be merged', 405)
+          ::MergeRequests::MergeService.new(merge_request.target_project, current_user, merge_params).
+            execute(merge_request)
         end
 
         present merge_request, with: Entities::MergeRequest
@@ -233,15 +229,9 @@ module API
 
         # Merge request can not be merged
         # because user dont have permissions to push into target branch
-        unauthorized! unless allowed
+        unauthorized! unless merge_request.can_cancel_merge_when_build_succeeds?(current_user)
 
-        if merge_request.merged? || !merge_request.open? || !merge_request.merge_when_build_succeeds?
-          not_allowed!
-        end
-
-        merge_request.reset_merge_when_build_succeeds
-
-        present merge_request, with: Entities::MergeRequest
+        ::MergeRequest::MergeWhenBuildSucceedsService.new(merge_request.target_project, current_user).cancel(merge_request)
       end
 
       # Get a merge request's comments