Incorporate feedback

14 files changed, 134 insertions, 83 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 131dfda6b5f..1f81e23c7a3 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -2,25 +2,28 @@
 #
 # Table name: merge_requests
 #
-#  id                :integer          not null, primary key
-#  target_branch     :string(255)      not null
-#  source_branch     :string(255)      not null
-#  source_project_id :integer          not null
-#  author_id         :integer
-#  assignee_id       :integer
-#  title             :string(255)
-#  created_at        :datetime
-#  updated_at        :datetime
-#  milestone_id      :integer
-#  state             :string(255)
-#  merge_status      :string(255)
-#  target_project_id :integer          not null
-#  iid               :integer
-#  description       :text
-#  position          :integer          default(0)
-#  locked_at         :datetime
-#  updated_by_id     :integer
-#  merge_error       :string(255)
+#  id                         :integer          not null, primary key
+#  target_branch              :string(255)      not null
+#  source_branch              :string(255)      not null
+#  source_project_id          :integer          not null
+#  author_id                  :integer
+#  assignee_id                :integer
+#  title                      :string(255)
+#  created_at                 :datetime
+#  updated_at                 :datetime
+#  milestone_id               :integer
+#  state                      :string(255)
+#  merge_status               :string(255)
+#  target_project_id          :integer          not null
+#  iid                        :integer
+#  description                :text
+#  position                   :integer          default(0)
+#  locked_at                  :datetime
+#  updated_by_id              :integer
+#  merge_error                :string(255)
+#  merge_params               :text (serialized to hash)
+#  merge_when_build_succeeds  :boolean          default(false), not null
+#  merge_user_id              :integer
 #
 
 require Rails.root.join("app/models/commit")
@@ -124,6 +127,7 @@ class MergeRequest < ActiveRecord::Base
   validates :source_branch, presence: true
   validates :target_project, presence: true
   validates :target_branch, presence: true
+  validates :merge_user, presence: true, if: :merge_when_build_succeeds?
   validate :validate_branches
   validate :validate_fork
 
@@ -496,8 +500,6 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def ci_commit
-    if last_commit
-      source_project.ci_commit(last_commit.id)
-    end
+    @ci_commit ||= source_project.ci_commit(last_commit.id) if last_commit
   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 2f101e53a3f..5cf7404a493 100644
--- a/app/services/merge_requests/merge_when_build_succeeds_service.rb
+++ b/app/services/merge_requests/merge_when_build_succeeds_service.rb
@@ -1,6 +1,6 @@
 module MergeRequests
   class MergeWhenBuildSucceedsService < MergeRequests::BaseService
-    # Marks the passed `merge_request` to be marked when the build succeeds or
+    # Marks the passed `merge_request` to be merged when the build succeeds or
     # updates the params for the automatic merge
     def execute(merge_request)
       merge_request.merge_params.merge!(params)
@@ -12,7 +12,7 @@ module MergeRequests
         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)
+        SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.last_commit)
       end
 
       merge_request.save
@@ -25,8 +25,7 @@ module MergeRequests
       merge_requests.each do |merge_request|
         next unless merge_request.merge_when_build_succeeds?
 
-        ci_commit = merge_request.ci_commit
-        if ci_commit && ci_commit.success? && merge_request.mergeable?
+        if merge_request.ci_commit && merge_request.ci_commit.success? && merge_request.mergeable?
           MergeWorker.perform_async(merge_request.id, merge_request.merge_user_id, merge_request.merge_params)
         end
       end
@@ -34,7 +33,7 @@ module MergeRequests
 
     # Cancels the automatic merge
     def cancel(merge_request)
-      if merge_request.merge_when_build_succeeds? && merge_request.open? && !merge_request.merged?
+      if merge_request.merge_when_build_succeeds? && merge_request.open?
         merge_request.reset_merge_when_build_succeeds
         SystemNoteService.cancel_merge_when_build_succeeds(merge_request, @project, @current_user)
 
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index ed557fef814..f84e480ca9c 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -131,15 +131,15 @@ class SystemNoteService
   end
 
   # Called when 'merge when build succeeds' is executed
-  def self.merge_when_build_succeeds(noteable, project, author, ci_commit)
-    body = "Enabled an automatic merge when the build for #{ci_commit.sha} succeeds"
+  def self.merge_when_build_succeeds(noteable, project, author, last_commit)
+    body = "Enabled an automatic merge when the build for #{last_commit.to_reference} succeeds"
 
     create_note(noteable: noteable, project: project, author: author, note: body)
   end
 
   # Called when 'merge when build succeeds' is canceled
   def self.cancel_merge_when_build_succeeds(noteable, project, author)
-    body = "Canceled the automatic merge"
+    body = "Cancelled the automatic merge"
 
     create_note(noteable: noteable, project: project, author: author, note: body)
   end
diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml
index c5234c0618c..d10ccb30571 100644
--- a/app/views/projects/merge_requests/_merge_request.html.haml
+++ b/app/views/projects/merge_requests/_merge_request.html.haml
@@ -1,4 +1,3 @@
-- ci_commit = merge_request.ci_commit
 %li{ class: mr_css_classes(merge_request) }
   .merge-request-title
     %span.merge-request-title-text
@@ -7,8 +6,8 @@
       - merge_request.labels.each do |label|
         = link_to_label(label, project: merge_request.project)
     .pull-right.light
-      - if ci_commit
-        = render_ci_status(ci_commit)
+      - if merge_request.ci_commit
+        = render_ci_status(merge_request.ci_commit)
       - if merge_request.merged?
         %span
           %i.fa.fa-check
diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml
index ba5ad22bca7..9513a18f105 100644
--- a/app/views/projects/merge_requests/widget/_heading.html.haml
+++ b/app/views/projects/merge_requests/widget/_heading.html.haml
@@ -1,13 +1,12 @@
-- ci_commit = @merge_request.ci_commit
-- if ci_commit
-  - status = ci_commit.status
+- if @merge_request.ci_commit
+  - status = @merge_request.ci_commit.status
   .mr-widget-heading
     .ci_widget{class: "ci-#{status}"}
-      = ci_status_icon(ci_commit)
+      = ci_status_icon(@merge_request.ci_commit)
       %span CI build #{status}
       for #{@merge_request.last_commit_short_sha}.
       %span.ci-coverage
-      = link_to "View build details", ci_status_path(ci_commit)
+      = link_to "View build details", ci_status_path(@merge_request.ci_commit)
 
 - elsif @merge_request.has_ci?
   - # Compatibility with old CI integrations (ex jenkins) when you request status from CI server via AJAX
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 279e2ec91f8..f7d872aa455 100644
--- a/app/views/projects/merge_requests/widget/open/_accept.html.haml
+++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml
@@ -4,8 +4,7 @@
   = hidden_field_tag :authenticity_token, form_authenticity_token
   .accept-merge-holder.clearfix.js-toggle-container
     .accept-action
-      - ci_commit = @merge_request.ci_commit
-      - if ci_commit && ci_commit.active?
+      - if @merge_request.ci_commit && @merge_request.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
         = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do
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 43ba49c5a5e..9788d68b54e 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,27 +1,27 @@
 %h4
-  Approved by #{link_to_member(@project, @merge_request.merge_user, avatar: true)}
+  Set 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.
 %div
-  - source_branch_removed = @merge_request.merge_params["should_remove_source_branch"].present?
-  - if source_branch_removed
+  - should_remove_source_branch = @merge_request.merge_params["should_remove_source_branch"].present?
+  %p
     = 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
+    - if should_remove_source_branch
+      The source branch will be removed.
+    - else
       The source branch will not be removed.
 
-  - if (@merge_request.can_remove_source_branch?(current_user) && !source_branch_removed) || @merge_request.can_cancel_merge_when_build_succeeds?(current_user)
+  - remove_source_branch_button = @merge_request.can_remove_source_branch?(current_user) && !should_remove_source_branch
+  - user_can_cancel_automatic_merge = @merge_request.can_cancel_merge_when_build_succeeds?(current_user)
+  - if remove_source_branch_button || user_can_cancel_automatic_merge
     .clearfix.prepend-top-10
-      - if @merge_request.can_remove_source_branch?(current_user) && !source_branch_removed
+      - if remove_source_branch_button
         = 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_cancel_merge_when_build_succeeds?(current_user)
+
+      - if user_can_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
diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb
index 729a49c9f72..5b4d7f41bc4 100644
--- a/spec/factories/merge_requests.rb
+++ b/spec/factories/merge_requests.rb
@@ -65,6 +65,11 @@ FactoryGirl.define do
       target_branch "master"
     end
 
+    trait :merge_when_build_succeeds do
+      merge_when_build_succeeds true
+      merge_user author
+    end
+
     factory :closed_merge_request, traits: [:closed]
     factory :reopened_merge_request, traits: [:reopened]
     factory :merge_request_with_diffs, traits: [:with_diffs]
diff --git a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb
index b25a3f05e29..2e64e903d1e 100644
--- a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb
+++ b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb
@@ -1,4 +1,6 @@
 require 'spec_helper'
+# rubocop:disable Lint/UselessAssignment
+# As rubocop doesn't see a need for both `ci_commit` and `ci_build`
 
 feature 'Merge When Build Succeeds', feature: true, js: true do
   let(:user) { create(:user) }
@@ -32,16 +34,20 @@ feature 'Merge When Build Succeeds', feature: true, js: true do
       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 "Set by #{user.name} to be merged automatically when the build succeeds."
         expect(page).to have_content "The source branch will not be removed."
+
+        visit_merge_request(merge_request) # Needed to refresh the page
+        expect(page).to have_content /Enabled an automatic merge when the build for [0-9a-f]{8} succeeds/i
       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) }
+    let(:merge_request) do
+      create(:merge_request_with_diffs, source_project: project, author: user,
+                                        merge_user: user, title: "MepMep", merge_when_build_succeeds: true)
+    end
 
     before do
       merge_request.source_project.team << [user, :master]
@@ -60,10 +66,16 @@ feature 'Merge When Build Succeeds', feature: true, js: true do
       click_link "Cancel Automatic Merge"
 
       expect(page).to have_button  "Merge When Build Succeeds"
+
+      visit_merge_request(merge_request) # Needed to refresh the page
+      expect(page).to have_content "Cancelled the automatic merge"
     end
 
     it "allows the user to remove the source branch" do
       expect(page).to have_link "Remove Source Branch When Merged"
+
+      click_link "Remove Source Branch When Merged"
+      expect(page).to have_content "The source branch will be removed"
     end
   end
 
@@ -78,3 +90,4 @@ feature 'Merge When Build Succeeds', feature: true, js: true do
     visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request)
   end
 end
+# rubocop:enable Lint/UselessAssignment
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index c7a9765825e..63e1cd1fb92 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -48,6 +48,24 @@ describe MergeRequest do
   describe 'validation' do
     it { is_expected.to validate_presence_of(:target_branch) }
     it { is_expected.to validate_presence_of(:source_branch) }
+
+    context "Validation of merge user with Merge When Build succeeds" do
+      it "allows user to be nil when the feature is disabled" do
+        expect(subject).to be_valid
+      end
+
+      it "is invalid without merge user" do
+        subject.merge_when_build_succeeds = true
+        expect(subject).not_to be_valid
+      end
+
+      it "is valid with merge user" do
+        subject.merge_when_build_succeeds = true
+        subject.merge_user = build(:user)
+
+        expect(subject).to be_valid
+      end
+    end
   end
 
   describe 'respond to' do
@@ -175,31 +193,41 @@ describe MergeRequest do
   end
 
   describe '#can_remove_source_branch' do
-    let(:user) { build(:user)}
+    let(:user) { create(:user) }
+    let(:user2) { create(: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 = [];
+      subject.source_branch = "feature"
+      subject.target_branch = "master"
+      subject.save!
+    end
 
+    it "can't be removed when its a protected branch" do
+      allow(subject.source_project).to receive(:protected_branch?).and_return(true)
       expect(subject.can_remove_source_branch?(user)).to be_falsey
     end
 
     it "cant remove a root ref" do
-      subject.source_branch = "master";
+      subject.source_branch = "master"
+      subject.target_branch = "feature"
 
       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))
+    it "is unable to remove the source branch for a project the user cannot push to" do
+      expect(subject.can_remove_source_branch?(user2)).to be_falsey
+    end
+
+    it "is can be removed in all other cases" do
+      expect(subject.can_remove_source_branch?(user)).to be_truthy
     end
   end
 
   describe "#reset_merge_when_build_succeeds" do
-    let(:merge_if_green) { create :merge_request, merge_when_build_succeeds: true }
+    let(:merge_if_green) { create :merge_request, merge_when_build_succeeds: true, merge_user: create(:user) }
+
     it "sets the item to false" do
       merge_if_green.reset_merge_when_build_succeeds
       merge_if_green.reload
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 91ae2059e95..6b7a066ac40 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -303,7 +303,7 @@ 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) }
+    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)
diff --git a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb
index 8638539173b..a62d88cde86 100644
--- a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb
+++ b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb
@@ -3,37 +3,38 @@ 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(:mr_merge_if_green_enabled) do
+    create(:merge_request, merge_when_build_succeeds: true, merge_user: user,
+                           source_branch: "source_branch", target_branch: project.default_branch,
+                           source_project: project, target_project: project, state: "opened")
+  end
+
   let(:project) { create(:project) }
+  let(:ci_commit) { create(:ci_commit_with_one_job, ref: mr_merge_if_green_enabled.source_branch, gl_project: 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)
+        service.execute(merge_request)
       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
+      end
 
+      it 'creates a system note' do
         note = merge_request.notes.last
-        expect(note.note).to include 'Enabled an automatic merge when the build for'
+        expect(note.note).to match /Enabled an automatic merge when the build for (\w+\/\w+@)?[0-9a-z]{8}/
       end
     end
 
-    context 'allready approved' do
+    context 'already 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) }
 
@@ -74,5 +75,10 @@ describe MergeRequests::MergeWhenBuildSucceedsService do
       expect(mr_merge_if_green_enabled.merge_params).to eq({})
       expect(mr_merge_if_green_enabled.merge_user).to be nil
     end
+
+    it 'Posts a system note' do
+      note = mr_merge_if_green_enabled.notes.last
+      expect(note.note).to include 'Cancelled the automatic merge'
+    end
   end
 end
diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb
index 18b2659c1f6..9a8174f95fd 100644
--- a/spec/services/merge_requests/refresh_service_spec.rb
+++ b/spec/services/merge_requests/refresh_service_spec.rb
@@ -18,7 +18,8 @@ describe MergeRequests::RefreshService do
                               source_branch: 'master',
                               target_branch: 'feature',
                               target_project: @project,
-                              merge_when_build_succeeds: true)
+                              merge_when_build_succeeds: true,
+                              merge_user: @user)
 
       @fork_merge_request = create(:merge_request,
                                    source_project: @fork_project,
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 5d41a5cdc69..333035f2d2c 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -208,20 +208,20 @@ describe SystemNoteService do
   end
 
   describe '.merge_when_build_succeeds' do
-    let(:ci_commit) { create :ci_commit_without_jobs }
+    let(:ci_commit) { build :ci_commit_without_jobs }
     let(:noteable) { create :merge_request }
 
-    subject { described_class.merge_when_build_succeeds(noteable, project, author, ci_commit) }
+    subject { described_class.merge_when_build_succeeds(noteable, project, author, noteable.last_commit) }
 
     it_behaves_like 'a system note'
 
     it "posts the Merge When Build Succeeds system note" do
-      expect(subject.note).to eq  "Enabled an automatic merge when the build for 97de212e80737a608d939f648d959671fb0a0142 succeeds"
+      expect(subject.note).to match  /Enabled an automatic merge when the build for (\w+\/\w+@)?[0-9a-f]{40} succeeds/
     end
   end
 
   describe '.cancel_merge_when_build_succeeds' do
-    let(:ci_commit) { create :ci_commit_without_jobs }
+    let(:ci_commit) { build :ci_commit_without_jobs }
     let(:noteable) { create :merge_request }
 
     subject { described_class.cancel_merge_when_build_succeeds(noteable, project, author) }
@@ -229,7 +229,7 @@ describe SystemNoteService do
     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"
+      expect(subject.note).to eq  "Cancelled the automatic merge"
     end
   end