Specs for 'Merge When Build Succeeds'

.gitignore

@@ -38,6 +38,7 @@ public/assets/
 public/uploads.*
 public/uploads/
 shared/artifacts/
+TODO
 rails_best_practices_output.html
 /tags
 tmp/

app/models/merge_request.rb

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

app/services/merge_requests/merge_when_build_succeeds_service.rb

@@ -11,13 +11,11 @@ module MergeRequests
       unless already_approved
         merge_request.merge_when_build_succeeds = true
         merge_request.merge_user                = @current_user
+
+        SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.ci_commit)
       end
 
       merge_request.save
-
-      unless already_approved
-        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

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, ci_commit)
-    body = "Approved an automatic merge when the build for #{ci_commit.sha} succeeds"
+    body = "Enabled 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/_merged.html.haml

@@ -13,7 +13,7 @@
           %span.label-branch= @merge_request.target_branch
         The source branch has been removed.
 
-      - elsif can_remove_branch?(@merge_request.source_project, @merge_request.source_branch)
+      - elsif @merge_request.can_remove_source_branch?(current_user)
         .remove_source_branch_widget
           %p
             = succeed '.' do
@@ -48,5 +48,3 @@
             $('.remove_source_branch_in_progress').hide();
             $('.remove_source_branch_widget.failed').show();
           });
-
-

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

@@ -7,13 +7,13 @@
       - ci_commit = @merge_request.ci_commit
       - if ci_commit && ci_commit.active?
         = f.button class: "btn btn-create btn-grouped merge_when_build_succeeds", name: "merge_when_build_succeeds" do
-          Merge when Build Succeeds
+          Merge When Build Succeeds
         = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do
           Accept Merge Request Now
       - else
         = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do
           Accept Merge Request
-    - if @merge_request.can_remove_source_branch?
+    - if @merge_request.can_remove_source_branch?(current_user)
       .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

@@ -3,22 +3,25 @@
   to be merged automatically when
   #{link_to "the build", ci_status_path(@merge_request.ci_commit)} succeeds.
 %div
-  - if @merge_request.merge_params["should_remove_source_branch"].present?
+  - source_branch_removed = @merge_request.merge_params["should_remove_source_branch"].present?
+  - if source_branch_removed
     = succeed '.' do
       The changes will be merged into
       %span.label-branch= @merge_request.target_branch
     The source branch will be removed.
+  - else
     %p
       = succeed '.' do
         The changes will be merged into
         %span.label-branch= @merge_request.target_branch
       The source branch will not be removed.
 
-  .clearfix.prepend-top-10
+  - if (@merge_request.can_remove_source_branch?(current_user) && !source_branch_removed) || @merge_request.can_cancel_merge_when_build_succeeds?(current_user)
-    - if @merge_request.can_remove_source_branch? && !@merge_request.merge_params["should_remove_source_branch"].present?
+    .clearfix.prepend-top-10
-      = 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
+      - if @merge_request.can_remove_source_branch?(current_user) && !source_branch_removed
-        = icon('times')
+        = 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
-        Remove Source Branch When Merged
+          = icon('times')
-    - if @merge_request.merge_when_build_succeeds
+          Remove Source Branch When Merged
-      = 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
+      - if @merge_request.can_cancel_merge_when_build_succeeds?(current_user)
-        Cancel Automatic Merge
+        = 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

lib/api/entities.rb

@@ -169,6 +169,7 @@ module API
       expose :description
       expose :work_in_progress?, as: :work_in_progress
       expose :milestone, using: Entities::Milestone
+      expose :merge_when_build_succeeds
     end
 
     class MergeRequestChanges < MergeRequest

lib/api/merge_requests.rb

@@ -179,11 +179,11 @@ module API
       # Merge MR
       #
       # Parameters:
-      #   id (required)                         - The ID of a project
+      #   id (required)                           - The ID of a project
-      #   merge_request_id (required)           - ID of MR
+      #   merge_request_id (required)             - ID of MR
-      #   merge_commit_message (optional)       - Custom merge commit message
+      #   merge_commit_message (optional)         - Custom merge commit message
-      #   should_remove_source_branch           - When true, the source branch will be deleted if possible
+      #   should_remove_source_branch (optional)  - 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
+      #   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
       #
@@ -193,12 +193,11 @@ module API
         # Merge request can not be merged
         # because user dont have permissions to push into target branch
         unauthorized! unless merge_request.can_be_merged_by?(current_user)
-
+        not_allowed! if !merge_request.open? || merge_request.work_in_progress?
-        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?
+        render_api_error!('Branch cannot be merged', 406) unless merge_request.can_be_merged?
 
         merge_params = {
           commit_message: params[:merge_commit_message],
@@ -206,7 +205,7 @@ module API
         }
 
         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).
+          ::MergeRequests::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).
@@ -224,11 +223,6 @@ module API
       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 merge_request.can_cancel_merge_when_build_succeeds?(current_user)
 
         ::MergeRequest::MergeWhenBuildSucceedsService.new(merge_request.target_project, current_user).cancel(merge_request)

spec/factories/ci/commits.rb

@@ -2,17 +2,18 @@
 #
 # Table name: commits
 #
-#  id           :integer          not null, primary key
+#  id             :integer          not null, primary key
-#  project_id   :integer
+#  project_id     :integer
-#  ref          :string(255)
+#  ref            :string(255)
-#  sha          :string(255)
+#  sha            :string(255)
-#  before_sha   :string(255)
+#  before_sha     :string(255)
-#  push_data    :text
+#  push_data      :text
-#  created_at   :datetime
+#  created_at     :datetime
-#  updated_at   :datetime
+#  updated_at     :datetime
-#  tag          :boolean          default(FALSE)
+#  tag            :boolean          default(FALSE)
-#  yaml_errors  :text
+#  yaml_errors    :text
-#  committed_at :datetime
+#  committed_at   :datetime
+#  gl_project_id  :integer
 #
 
 # Read about factories at https://github.com/thoughtbot/factory_girl

spec/factories/commit_statuses.rb

@@ -5,7 +5,7 @@ FactoryGirl.define do
     name 'default'
     status 'success'
     description 'commit status'
-    commit factory: :ci_commit
+    commit factory: :ci_commit_with_one_job
 
     factory :generic_commit_status, class: GenericCommitStatus do
       name 'generic'

spec/features/merge_requests/merge_when_build_succeeds_spec.rb

@@ -0,0 +1,80 @@
+require 'spec_helper'
+
+feature 'Merge When Build Succeeds', feature: true, js: true do
+  let(:user) { create(:user) }
+
+  let(:project)       { create(:project, :public) }
+  let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user, title: "Bug NS-04") }
+
+  before do
+    project.team << [user, :master]
+    project.enable_ci
+  end
+
+  context "Active build for Merge Request" do
+    before do
+      ci_commit = create(:ci_commit, gl_project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch)
+      ci_build = create(:ci_build, commit: ci_commit)
+
+      login_as user
+      visit_merge_request(merge_request)
+    end
+
+    it 'displays the Merge When Build Succeeds button' do
+      expect(page).to have_button "Merge When Build Succeeds"
+    end
+
+    context "Merge When Build succeeds enabled" do
+      before do
+        click_button "Merge When Build Succeeds"
+      end
+
+      it 'activates Merge When Build Succeeds feature' do
+        expect(page).to have_link "Cancel Automatic Merge"
+
+        expect(page).to have_content "Approved by #{user.name} to be merged automatically when the build succeeds."
+        expect(page).to have_content "The source branch will not be removed."
+      end
+    end
+  end
+
+  context 'When it is enabled' do
+    # No clue how, but push a new commit to the branch
+    let(:merge_request) { create(:merge_request_with_diffs, source_project: project, # source_branch: "mepmep",
+                                  author: user, title: "Bug NS-04", merge_when_build_succeeds: true) }
+
+    before do
+      merge_request.source_project.team << [user, :master]
+      merge_request.source_branch = "feature"
+      merge_request.target_branch = "master"
+      merge_request.save!
+
+      ci_commit = create(:ci_commit, gl_project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch)
+      ci_build = create(:ci_build, commit: ci_commit)
+
+      login_as user
+      visit_merge_request(merge_request)
+    end
+
+    it 'cancels the automatic merge' do
+      click_link "Cancel Automatic Merge"
+
+      expect(page).to have_button  "Merge When Build Succeeds"
+    end
+
+    it "allows the user to remove the source branch" do
+      expect(page).to have_link "Remove Source Branch When Merged"
+    end
+  end
+
+  context 'Build is not active' do
+    it "should not allow for enabling" do
+      visit_merge_request(merge_request)
+      expect(page).not_to have_button "Merge When Build Succeeds"
+    end
+  end
+
+  def visit_merge_request(merge_request)
+    visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request)
+  end
+end

spec/models/merge_request_spec.rb

@@ -174,6 +174,30 @@ describe MergeRequest do
     end
   end
 
+  describe '#can_remove_source_branch' do
+    let(:user) { build(:user)}
+
+    before do
+      subject.source_project.team << [user, :master]
+    end
+
+    it "cant be merged when its a a protected branch" do
+      subject.source_project.protected_branches = [];
+
+      expect(subject.can_remove_source_branch?(user)).to be_falsey
+    end
+
+    it "cant remove a root ref" do
+      subject.source_branch = "master";
+
+      expect(subject.can_remove_source_branch?(user)).to be_falsey
+    end
+
+    it "is truthy in all other cases" do
+      expect(subject.can_remove_source_branch?(user))
+    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

spec/requests/api/merge_requests_spec.rb

@@ -303,19 +303,21 @@ describe API::API, api: true  do
   end
 
   describe "PUT /projects/:id/merge_request/:merge_request_id/merge" do
+    let (:ci_commit) { create(:ci_commit_without_jobs) }
+
     it "should return merge_request in case of success" do
       put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user)
 
       expect(response.status).to eq(200)
     end
 
-    it "should return 405 if branch can't be merged" do
+    it "should return 406 if branch can't be merged" do
       allow_any_instance_of(MergeRequest).
         to receive(:can_be_merged?).and_return(false)
 
       put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user)
 
-      expect(response.status).to eq(405)
+      expect(response.status).to eq(406)
       expect(json_response['message']).to eq('Branch cannot be merged')
     end
 
@@ -340,6 +342,17 @@ describe API::API, api: true  do
       expect(response.status).to eq(401)
       expect(json_response['message']).to eq('401 Unauthorized')
     end
+
+    it "enables merge when build succeeds if the ci is active" do
+      allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_return(ci_commit)
+      allow(ci_commit).to receive(:active?).and_return(true)
+
+      put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user), merge_when_build_succeeds: true
+
+      expect(response.status).to eq(200)
+      expect(json_response['title']).to eq('Test')
+      expect(json_response['merge_when_build_succeeds']).to eq(true)
+    end
   end
 
   describe "PUT /projects/:id/merge_request/:merge_request_id" do

spec/services/merge_requests/merge_service_spec.rb

@@ -13,12 +13,12 @@ describe MergeRequests::MergeService do
 
   describe :execute do
     context 'valid params' do
-      let(:service) { MergeRequests::MergeService.new(project, user, {}) }
+      let(:service) { MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') }
 
       before do
         allow(service).to receive(:execute_hooks)
 
-        service.execute(merge_request, 'Awesome message')
+        service.execute(merge_request)
       end
 
       it { expect(merge_request).to be_valid }
@@ -37,14 +37,14 @@ describe MergeRequests::MergeService do
     end
 
     context "error handling" do
-      let(:service) { MergeRequests::MergeService.new(project, user, {}) }
+      let(:service) { MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') }
 
       it 'saves error if there is an exception' do
         allow(service).to receive(:repository).and_raise("error")
 
         allow(service).to receive(:execute_hooks)
 
-        service.execute(merge_request, 'Awesome message')
+        service.execute(merge_request)
 
         expect(merge_request.merge_error).to eq("Something went wrong during merge")
       end

spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb

@@ -0,0 +1,78 @@
+require 'spec_helper'
+
+describe MergeRequests::MergeWhenBuildSucceedsService do
+  let(:user)          { create(:user) }
+  let(:merge_request) { create(:merge_request) }
+  let(:mr_merge_if_green_enabled) { create(:merge_request, merge_when_build_succeeds: true,
+                                          source_branch: "source_branch", target_branch: project.default_branch,
+                                          source_project: project, target_project: project, state: "opened") }
+  let(:ci_commit) { create(:ci_commit_with_one_job, ref: mr_merge_if_green_enabled.source_branch) }
+  let(:project) { create(:project) }
+  let(:service) { MergeRequests::MergeWhenBuildSucceedsService.new(project, user, commit_message: 'Awesome message') }
+
+  before do
+    project.ci_commits = [ci_commit]
+    project.save!
+  end
+  describe "#execute" do
+    context 'first time enabling' do
+      before do
+        allow(merge_request).to receive(:ci_commit).and_return(ci_commit)
+      end
+
+      it 'sets the params, merge_user, and flag' do
+        service.execute(merge_request)
+
+        expect(merge_request).to be_valid
+        expect(merge_request.merge_when_build_succeeds).to be_truthy
+        expect(merge_request.merge_params).to eq commit_message: 'Awesome message'
+        expect(merge_request.merge_user).to be user
+
+        note = merge_request.notes.last
+        expect(note.note).to include 'Enabled an automatic merge when the build for'
+      end
+    end
+
+    context 'allready approved' do
+      let(:service) { MergeRequests::MergeWhenBuildSucceedsService.new(project, user, new_key: true) }
+      let(:build)   { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch) }
+
+      before do
+        allow(mr_merge_if_green_enabled).to receive(:ci_commit).and_return(ci_commit)
+        allow(mr_merge_if_green_enabled).to receive(:mergeable?).and_return(true)
+        allow(ci_commit).to receive(:success?).and_return(true)
+      end
+
+      it 'updates the merge params' do
+        expect(SystemNoteService).not_to receive(:merge_when_build_succeeds)
+
+        service.execute(mr_merge_if_green_enabled)
+        expect(mr_merge_if_green_enabled.merge_params).to have_key(:new_key)
+      end
+    end
+  end
+
+  describe "#trigger" do
+    let(:build)     { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch, status: "success") }
+
+    it "merges all merge requests with merge when build succeeds enabled" do
+      allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_return(ci_commit)
+      allow(ci_commit).to receive(:success?).and_return(true)
+
+      expect(MergeWorker).to receive(:perform_async)
+      service.trigger(build)
+    end
+  end
+
+  describe "#cancel" do
+    before do
+      service.cancel(mr_merge_if_green_enabled)
+    end
+
+    it "resets all the merge_when_build_succeeds params" do
+      expect(mr_merge_if_green_enabled.merge_when_build_succeeds).to be_falsey
+      expect(mr_merge_if_green_enabled.merge_params).to eq({})
+      expect(mr_merge_if_green_enabled.merge_user).to be nil
+    end
+  end
+end

spec/services/merge_requests/refresh_service_spec.rb

@@ -17,7 +17,8 @@ describe MergeRequests::RefreshService do
                               source_project: @project,
                               source_branch: 'master',
                               target_branch: 'feature',
-                              target_project: @project)
+                              target_project: @project,
+                              merge_when_build_succeeds: true)
 
       @fork_merge_request = create(:merge_request,
                                    source_project: @fork_project,
@@ -46,6 +47,7 @@ describe MergeRequests::RefreshService do
 
       it { expect(@merge_request.notes).not_to be_empty }
       it { expect(@merge_request).to be_open }
+      it { expect(@merge_request.merge_when_build_succeeds).to be_falsey}
       it { expect(@fork_merge_request).to be_open }
       it { expect(@fork_merge_request.notes).to be_empty }
     end
@@ -146,6 +148,7 @@ describe MergeRequests::RefreshService do
       end
     end
 
+
     def reload_mrs
       @merge_request.reload
       @fork_merge_request.reload

spec/services/system_note_service_spec.rb

@@ -208,18 +208,28 @@ describe SystemNoteService do
   end
 
   describe '.merge_when_build_succeeds' do
-    let(:ci_commit) { create :ci_commit, gl_project: project }
+    let(:ci_commit) { create :ci_commit_without_jobs }
-    let(:merge_request) { create :merge_request, project: project }
+    let(:noteable) { create :merge_request }
 
-    subject { described_class.merge_when_build_succeeds(merge_request, project, author) }
+    subject { described_class.merge_when_build_succeeds(noteable, project, author, ci_commit) }
 
     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)
+      expect(subject.note).to eq  "Enabled an automatic merge when the build for 97de212e80737a608d939f648d959671fb0a0142 succeeds"
-      allow(ci_commit).to receive(:short_sha).and_return('12345678')
+    end
+  end
 
-      expect(subject.note).to eq  "This merge request will be automatically merged when the build for 12345678 succeeds"
+  describe '.cancel_merge_when_build_succeeds' do
+    let(:ci_commit) { create :ci_commit_without_jobs }
+    let(:noteable) { create :merge_request }
+
+    subject { described_class.cancel_merge_when_build_succeeds(noteable, project, author) }
+
+    it_behaves_like 'a system note'
+
+    it "posts the Merge When Build Succeeds system note" do
+      expect(subject.note).to eq  "Canceled the automatic merge"
     end
   end