API support, incorporated feedback

15 files changed, 157 insertions, 63 deletions

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index d58dab2d666..931298df5d8 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -171,9 +171,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @merge_request.update(merge_error: nil)
 
     if params[:merge_when_build_succeeds] && @merge_request.ci_commit.active?
-      MergeRequests::MergeWhenBuildSucceedsService.new(@project,
-                                                      current_user,
-                                                      merge_params: merge_params)
+      MergeRequests::MergeWhenBuildSucceedsService.new(@project, current_user, merge_params)
                                                       .execute(@merge_request)
       @status = :merge_when_build_succeeds
     else
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index b1049fab788..acc86b1a8cd 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -75,7 +75,7 @@ class CommitStatus < ActiveRecord::Base
       build.update_attributes finished_at: Time.now
     end
 
-    after_transition running: :success do |build, transition|
+    after_transition [:pending, :running] => :success do |build, transition|
       MergeRequests::MergeWhenBuildSucceedsService.new(build.commit.gl_project, nil).trigger(build)
     end
 
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index c5f04dbcf4c..7b372399a3a 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -253,6 +253,10 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
+  def mergeable_by_or_author(user)
+    self.can_be_merged_by?(user) || self.author == user
+  end
+
   def mr_and_commit_notes
     # Fetch comments only from last 100 commits
     commits_for_notes_limit = 100
@@ -390,7 +394,7 @@ class MergeRequest < ActiveRecord::Base
 
   def reset_merge_when_build_succeeds
     return unless merge_when_build_succeeds?
-    
+
     self.merge_when_build_succeeds = false
     self.merge_user = nil
     self.merge_params = nil
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index 7963af127e1..db8d18a7d84 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -6,15 +6,12 @@ module MergeRequests
   # Executed when you do merge via GitLab UI
   #
   class MergeService < MergeRequests::BaseService
-    attr_reader :merge_request, :commit_message
+    attr_reader :merge_request
 
-    def execute(merge_request, commit_message)
-      @commit_message = commit_message
+    def execute(merge_request)
       @merge_request = merge_request
 
-      unless @merge_request.mergeable?
-        return error('Merge request is not mergeable')
-      end
+      return error('Merge request is not mergeable') unless @merge_request.mergeable?
 
       merge_request.in_locked_state do
         if commit
@@ -32,7 +29,7 @@ module MergeRequests
       committer = repository.user_to_committer(current_user)
 
       options = {
-        message: commit_message,
+        message: params[:commit_message] || merge_request.merge_commit_message,
         author: committer,
         committer: committer
       }
@@ -46,6 +43,11 @@ module MergeRequests
 
     def after_merge
       MergeRequests::PostMergeService.new(project, current_user).execute(merge_request)
+
+      if params[:should_remove_source_branch]
+        DeleteBranchService.new(@merge_request.source_project, current_user).
+          execute(merge_request.source_branch)
+      end
     end
   end
 end
diff --git a/app/services/merge_requests/merge_when_build_succeeds_service.rb b/app/services/merge_requests/merge_when_build_succeeds_service.rb
index a4418360b8c..d5cae2f98f7 100644
--- a/app/services/merge_requests/merge_when_build_succeeds_service.rb
+++ b/app/services/merge_requests/merge_when_build_succeeds_service.rb
@@ -1,7 +1,7 @@
 module MergeRequests
   class MergeWhenBuildSucceedsService < MergeRequests::BaseService
     def execute(merge_request)
-      merge_request.merge_params.merge!(params[:merge_params])
+      merge_request.merge_params.merge!(params)
 
       # The service is also called when the merge params are updated.
       already_approved = merge_request.merge_when_build_succeeds?
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index 335ef32abce..e5024857201 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -77,9 +77,7 @@ module MergeRequests
     end
 
     def reset_merge_when_build_succeeds
-      merge_requests_for_source_branch.each do |merge_request|
-        merge_request.reset_merge_when_build_succeeds
-      end
+      merge_requests_for_source_branch.each(&:reset_merge_when_build_succeeds)
     end
 
     def find_new_commits
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index c9846e9f26f..5e8281a3fd0 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -132,7 +132,7 @@ class SystemNoteService
 
   # Called when 'merge when build succeeds' is executed
   def self.merge_when_build_succeeds(noteable, project, author)
-    body = "Approved this request to be merged automatically when the build succeeds"
+    body = "This merge request will be automatically merged when the build for #{noteable.ci_commit.short_sha} succeeds"
 
     create_note(noteable: noteable, project: project, author: author, note: body)
   end
diff --git a/app/views/projects/merge_requests/widget/open/_accept.html.haml b/app/views/projects/merge_requests/widget/open/_accept.html.haml
index 2afc5f81251..2a51241971f 100644
--- a/app/views/projects/merge_requests/widget/open/_accept.html.haml
+++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml
@@ -29,5 +29,5 @@
     $('.accept_merge_request').on 'click', ->
       btn = $(this)
       btn.html("<i class='fa fa-spinner fa-spin'></i> Merge in progress")
-    $('.accept-mr-form').on 'ajax:sen', ->
+    $('.accept-mr-form').on 'ajax:send', ->
       $(".accept-mr-form :input").disable()
diff --git a/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml
index f3894334968..ddd1a7bd63d 100644
--- a/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml
+++ b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml
@@ -1,9 +1,9 @@
 %h4
   Approved by #{link_to_member(@project, @merge_request.merge_user, avatar: true)}
   to be merged automatically when
-  #{link_to "the build", ci_status_path(@merge_request.ci_commit)} succeeds
+  #{link_to "the build", ci_status_path(@merge_request.ci_commit)} succeeds.
 %div
-  - if @merge_request.merge_params["should_remove_source_branch"]
+  - if @merge_request.merge_params["should_remove_source_branch"].present?
     = succeed '.' do
       The changes will be merged into
       %span.label-branch= @merge_request.target_branch
@@ -19,9 +19,9 @@
 - if remove_source_branch_button || @merge_request.can_be_merged_by?(current_user)
   .clearfix.prepend-top-10
     - if remove_source_branch_button
-      = link_to merge_namespace_project_merge_request_path(@merge_request.source_project.namespace, @merge_request.source_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
+      = 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
-      = link_to merge_namespace_project_merge_request_path(@merge_request.source_project.namespace, @merge_request.source_project, @merge_request), remote: true, method: :delete, class: "btn btn-grouped btn-warning btn-sm" do
+      = 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
diff --git a/app/workers/merge_worker.rb b/app/workers/merge_worker.rb
index 5d1a8555b7d..c87c0a252b1 100644
--- a/app/workers/merge_worker.rb
+++ b/app/workers/merge_worker.rb
@@ -8,16 +8,7 @@ class MergeWorker
     current_user = User.find(current_user_id)
     merge_request = MergeRequest.find(merge_request_id)
 
-    result = MergeRequests::MergeService.new(merge_request.target_project, current_user).
-      execute(merge_request, params[:commit_message])
-
-    if result[:status] == :success && params[:should_remove_source_branch].present?
-      DeleteBranchService.new(merge_request.source_project, current_user).
-        execute(merge_request.source_branch)
-
-      merge_request.source_project.repository.expire_branch_names
-    end
-
-    result
+    MergeRequests::MergeService.new(merge_request.target_project, current_user, params).
+      execute(merge_request)
   end
 end
diff --git a/config/routes.rb b/config/routes.rb
index 917c3d3f1ed..993687665cc 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -556,8 +556,7 @@ Gitlab::Application.routes.draw do
             get :diffs
             get :commits
             post :merge
-            delete :merge, action: :cancel_merge_when_build_succeeds
-            get :merge_check
+            post :cancel_merge_when_build_succeeds
             get :ci_status
             post :toggle_subscription
           end
diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md
index ffa7f2cdf14..1b2ad1caf49 100644
--- a/doc/api/merge_requests.md
+++ b/doc/api/merge_requests.md
@@ -2,8 +2,8 @@
 
 ## List merge requests
 
-Get all merge requests for this project. 
-The `state` parameter can be used to get only merge requests with a given state (`opened`, `closed`, or `merged`) or all of them (`all`). 
+Get all merge requests for this project.
+The `state` parameter can be used to get only merge requests with a given state (`opened`, `closed`, or `merged`) or all of them (`all`).
 The pagination parameters `page` and `per_page` can be used to restrict the list of merge requests.
 
 ```
@@ -292,9 +292,59 @@ PUT /projects/:id/merge_request/:merge_request_id/merge
 
 Parameters:
 
-- `id` (required)                   - The ID of a project
-- `merge_request_id` (required)     - ID of MR
-- `merge_commit_message` (optional) - Custom merge commit message
+- `id` (required)                           - The ID of a project
+- `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
+
+```json
+{
+  "id": 1,
+  "target_branch": "master",
+  "source_branch": "test1",
+  "project_id": 3,
+  "title": "test1",
+  "state": "merged",
+  "upvotes": 0,
+  "downvotes": 0,
+  "author": {
+    "id": 1,
+    "username": "admin",
+    "email": "admin@example.com",
+    "name": "Administrator",
+    "state": "active",
+    "created_at": "2012-04-29T08:46:00Z"
+  },
+  "assignee": {
+    "id": 1,
+    "username": "admin",
+    "email": "admin@example.com",
+    "name": "Administrator",
+    "state": "active",
+    "created_at": "2012-04-29T08:46:00Z"
+  }
+}
+```
+
+## Cancel Merge When Build Succeeds
+
+Cancels the merge when build succeeds and reset the merge parameters
+
+If successfull 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'
+```
+PUT /projects/:id/merge_request/:merge_request_id/cancel_merge_when_build_succeeds
+```
+Parameters:
+
+- `id` (required)                           - The ID of a project
+- `merge_request_id` (required)             - ID of MR
 
 ```json
 {
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 6eb84baf9cb..f981432db36 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -179,9 +179,10 @@ module API
       # Merge MR
       #
       # Parameters:
-      #   id (required)                   - The ID of a project
-      #   merge_request_id (required)     - ID of MR
-      #   merge_commit_message (optional) - Custom merge commit message
+      #   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
       # Example:
       #   PUT /projects/:id/merge_request/:merge_request_id/merge
       #
@@ -191,34 +192,57 @@ module API
         allowed = ::Gitlab::GitAccess.new(current_user, user_project).
           can_push_to_branch?(merge_request.target_branch)
 
-        if allowed
-          if merge_request.unchecked?
-            merge_request.check_if_can_be_merged
-          end
+        # Merge request can not be merged
+        # because user dont have permissions to push into target branch
+        unauthorized! unless allowed
+
+        not_allowed! unless merge_request.open?
 
-          if merge_request.open? && !merge_request.work_in_progress?
-            if merge_request.can_be_merged?
-              commit_message = params[:merge_commit_message] || merge_request.merge_commit_message
+        merge_request.check_if_can_be_merged if merge_request.unchecked?
 
-              ::MergeRequests::MergeService.new(merge_request.target_project, current_user).
-                execute(merge_request, commit_message)
+        merge_params = {
+          commit_message: params[:merge_commit_message],
+          should_remove_source_branch: params[:should_remove_source_branch]
+        }
 
-              present merge_request, with: Entities::MergeRequest
-            else
-              render_api_error!('Branch cannot be merged', 405)
-            end
+        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
-            # Merge request can not be merged
-            # because it is already closed/merged or marked as WIP
-            not_allowed!
+            ::MergeRequests::MergeService.new(merge_request.target_project, current_user, merge_params).
+              execute(merge_request, merge_params)
           end
         else
-          # Merge request can not be merged
-          # because user dont have permissions to push into target branch
-          unauthorized!
+          render_api_error!('Branch cannot be merged', 405)
         end
+
+        present merge_request, with: Entities::MergeRequest
       end
 
+      # Cancel Merge if Merge When build succeeds is enabled
+      # Parameters:
+      #   id (required)                         - The ID of a project
+      #   merge_request_id (required)           - ID of MR
+      #
+      post ":id/merge_request/:merge_request_id/cancel_merge_when_build_succeeds" 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
+
+        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
+      end
 
       # Get a merge request's comments
       #
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index eed2cbc5412..e42534a8491 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -30,7 +30,7 @@ describe MergeRequest do
   describe 'associations' do
     it { is_expected.to belong_to(:target_project).with_foreign_key(:target_project_id).class_name('Project') }
     it { is_expected.to belong_to(:source_project).with_foreign_key(:source_project_id).class_name('Project') }
-
+    it { is_expected.to belong_to(:merge_user).class_name("User") }
     it { is_expected.to have_one(:merge_request_diff).dependent(:destroy) }
   end
 
@@ -53,6 +53,8 @@ describe MergeRequest do
     it { is_expected.to respond_to(:unchecked?) }
     it { is_expected.to respond_to(:can_be_merged?) }
     it { is_expected.to respond_to(:cannot_be_merged?) }
+    it { is_expected.to respond_to(:merge_params) }
+    it { is_expected.to respond_to(:merge_when_build_succeeds) }
   end
 
   describe '#to_reference' do
@@ -171,6 +173,16 @@ describe MergeRequest do
     end
   end
 
+  describe "#reset_merge_when_build_succeeds" do
+    let(:merge_if_green) { create :merge_request, merge_when_build_succeeds: true }
+    it "sets the item to false" do
+      merge_if_green.reset_merge_when_build_succeeds
+      merge_if_green.reload
+
+      expect(merge_if_green.merge_when_build_succeeds).to be_falsey
+    end
+  end
+
   describe "#hook_attrs" do
     it "has all the required keys" do
       attrs = subject.hook_attrs
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index a45130bd473..35912ece644 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -207,6 +207,22 @@ describe SystemNoteService do
     end
   end
 
+  describe '.merge_when_build_succeeds' do
+    let(:ci_commit) { create :ci_commit, gl_project: project }
+    let(:merge_request) { create :merge_request, project: project }
+
+    subject { described_class.merge_when_build_succeeds(merge_request, project, author) }
+
+    it_behaves_like 'a system note'
+
+    it "posts the Merge When Build Succeeds system note" do
+      allow(merge_request).to receive(:ci_commit).and_return(ci_commit)
+      allow(ci_commit).to receive(:short_sha).and_return('12345678')
+
+      expect(subject.note).to eq  "This merge request will be automatically merged when the build for 12345678 succeeds"
+    end
+  end
+
   describe '.change_title' do
     subject { described_class.change_title(noteable, project, author, 'Old title') }