Merge branch 'reset_approvals_on_push' into 'master'

Reset approvers on push



See merge request !440

CHANGELOG-EE

 v 7.13 (Unrelease)
   - Fix git hook validation on initial push to master branch.
+  - Reset approvals on push
 
 v 7.12.2
   - Fixed the alignment of project settings icons

app/services/merge_requests/refresh_service.rb

@@ -12,6 +12,7 @@ module MergeRequests
       reload_merge_requests
       execute_mr_web_hooks
       comment_mr_with_commits
+      reset_approvals_for_merge_requests
 
       true
     end
@@ -70,6 +71,20 @@ module MergeRequests
       end
     end
 
+    # Reset approvals for merge request
+    # Note: we should reset approvals for merge requests from forks too
+    def reset_approvals_for_merge_requests
+      if @project.approvals_before_merge.nonzero? && @project.reset_approvers_on_push
+        merge_requests = @project.merge_requests.opened.where(source_branch: @branch_name).to_a
+        merge_requests += @fork_merge_requests.where(source_branch: @branch_name).to_a
+        merge_requests = filter_merge_requests(merge_requests)
+
+        merge_requests.each do |merge_request|
+          merge_request.approvals.destroy_all
+        end
+      end
+    end
+
     # Add comment about pushing new commits to merge requests
     def comment_mr_with_commits
       merge_requests = @project.origin_merge_requests.opened.where(source_branch: @branch_name).to_a

app/views/projects/_merge_request_settings.html.haml

@@ -29,3 +29,10 @@
       = f.number_field :approvals_before_merge, class: "form-control", min: 0
       .help-block
         How many users should approve merge request before it can be accepted. 0 - approval is disabled
+
+  .form-group.reset-approvers-on-push
+    .col-sm-offset-2.col-sm-10
+      .checkbox
+        = f.label :reset_approvers_on_push do
+          = f.check_box :reset_approvers_on_push
+          %span.descr Reset approvers on push

db/migrate/20150709134649_add_reset_approvers_to_project.rb

+class AddResetApproversToProject < ActiveRecord::Migration
+  def change
+    add_column :projects, :reset_approvers_on_push, :boolean, default: true
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20150620233230) do
+ActiveRecord::Schema.define(version: 20150709134649) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -443,6 +443,7 @@ ActiveRecord::Schema.define(version: 20150620233230) do
     t.string   "import_source"
     t.boolean  "merge_requests_rebase_default", default: true
     t.integer  "approvals_before_merge",        default: 0,        null: false
+    t.boolean  "reset_approvers_on_push",       default: true
   end
 
   add_index "projects", ["created_at", "id"], name: "index_projects_on_created_at_and_id", using: :btree

spec/services/merge_requests/refresh_service_spec.rb

@@ -11,7 +11,7 @@ describe MergeRequests::RefreshService do
       group = create(:group)
       group.add_owner(@user)
 
-      @project = create(:project, namespace: group)
+      @project = create(:project, namespace: group, approvals_before_merge: 1, reset_approvers_on_push: true)
       @fork_project = Projects::ForkService.new(@project, @user).execute
       @merge_request = create(:merge_request,
                               source_project: @project,
@@ -25,6 +25,9 @@ describe MergeRequests::RefreshService do
                                    target_branch: 'feature',
                                    target_project: @project)
 
+      @merge_request.approvals.create(user_id: user.id)
+      @fork_merge_request.approvals.create(user_id: user.id)
+
       @commits = @merge_request.commits
 
       @oldrev = @commits.last.id
@@ -46,8 +49,10 @@ describe MergeRequests::RefreshService do
 
       it { expect(@merge_request.notes).not_to be_empty }
       it { expect(@merge_request).to be_open }
+      it { expect(@merge_request.approvals).to be_empty }
       it { expect(@fork_merge_request).to be_open }
       it { expect(@fork_merge_request.notes).to be_empty }
+      it { expect(@fork_merge_request.approvals).to be_empty }
     end
 
     context 'push to origin repo target branch' do
@@ -58,8 +63,10 @@ describe MergeRequests::RefreshService do
 
       it { expect(@merge_request.notes.last.note).to include('changed to merged') }
       it { expect(@merge_request).to be_merged }
+      it { expect(@merge_request.approvals).not_to be_empty }
       it { expect(@fork_merge_request).to be_merged }
       it { expect(@fork_merge_request.notes.last.note).to include('changed to merged') }
+      it { expect(@fork_merge_request.approvals).not_to be_empty }
     end
 
     context 'push to fork repo source branch' do
@@ -77,8 +84,10 @@ describe MergeRequests::RefreshService do
 
       it { expect(@merge_request.notes).to be_empty }
       it { expect(@merge_request).to be_open }
+      it { expect(@merge_request.approvals).not_to be_empty }
       it { expect(@fork_merge_request.notes.last.note).to include('Added 4 commits') }
       it { expect(@fork_merge_request).to be_open }
+      it { expect(@fork_merge_request.approvals).not_to be_empty }
     end
 
     context 'push to fork repo target branch' do
@@ -89,8 +98,10 @@ describe MergeRequests::RefreshService do
 
       it { expect(@merge_request.notes).to be_empty }
       it { expect(@merge_request).to be_open }
+      it { expect(@merge_request.approvals).not_to be_empty }
       it { expect(@fork_merge_request.notes).to be_empty }
       it { expect(@fork_merge_request).to be_open }
+      it { expect(@fork_merge_request.approvals).not_to be_empty }
     end
 
     context 'push to origin repo target branch after fork project was removed' do
@@ -102,8 +113,32 @@ describe MergeRequests::RefreshService do
 
       it { expect(@merge_request.notes.last.note).to include('changed to merged') }
       it { expect(@merge_request).to be_merged }
+      it { expect(@merge_request.approvals).not_to be_empty }
       it { expect(@fork_merge_request).to be_open }
       it { expect(@fork_merge_request.notes).to be_empty }
+      it { expect(@fork_merge_request.approvals).not_to be_empty }
+    end
+
+    context 'resetting approvals if they are enabled' do
+      it "does not reset approvals if approvals_before_merge si disabled" do
+        @project.update(approvals_before_merge: 0)
+        refresh_service = service.new(@project, @user)
+        allow(refresh_service).to receive(:execute_hooks)
+        refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+        reload_mrs
+
+        expect(@merge_request.approvals).not_to be_empty
+      end
+
+      it "does not reset approvals if reset_approvers_on_push si disabled" do
+        @project.update(reset_approvers_on_push: false)
+        refresh_service = service.new(@project, @user)
+        allow(refresh_service).to receive(:execute_hooks)
+        refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+        reload_mrs
+
+        expect(@merge_request.approvals).not_to be_empty
+      end
     end
 
     def reload_mrs