Merge branch '1457-reset-approvals-on-closed-mrs' into 'master'

"Reset approvals on push" also reset approvals of a closed merged request

Closes #1457

See merge request !1051

app/models/project.rb

@@ -121,8 +121,6 @@ class Project < ActiveRecord::Base
 
   # Merge Requests for target project should be removed with it
   has_many :merge_requests,     dependent: :destroy, foreign_key: 'target_project_id'
-  # Merge requests from source project should be kept when source project was removed
-  has_many :fork_merge_requests, foreign_key: 'source_project_id', class_name: 'MergeRequest'
   has_many :issues,             dependent: :destroy
   has_many :labels,             dependent: :destroy, class_name: 'ProjectLabel'
   has_many :services,           dependent: :destroy

app/services/merge_requests/base_service.rb

@@ -38,15 +38,13 @@ module MergeRequests
 
     private
 
-    def merge_requests_for(branch)
-      origin_merge_requests = @project.origin_merge_requests
-        .opened.where(source_branch: branch).to_a
-
-      fork_merge_requests = @project.fork_merge_requests
-        .opened.where(source_branch: branch).to_a
-
-      (origin_merge_requests + fork_merge_requests)
-        .uniq.select(&:source_project)
+    # Returns all origin and fork merge requests from `@project` satisfying passed arguments.
+    def merge_requests_for(source_branch, mr_states: [:opened])
+      MergeRequest
+        .with_state(mr_states)
+        .where(source_branch: source_branch, source_project_id: @project.id)
+        .preload(:source_project) # we don't need a #includes since we're just preloading for the #select
+        .select(&:source_project)
     end
 
     def pipeline_merge_requests(pipeline)

app/services/merge_requests/refresh_service.rb

@@ -43,7 +43,7 @@ module MergeRequests
         commit_ids.include?(merge_request.diff_head_sha)
       end
 
-      merge_requests.uniq.select(&:source_project).each do |merge_request|
+      filter_merge_requests(merge_requests).each do |merge_request|
         MergeRequests::PostMergeService.
           new(merge_request.target_project, @current_user).
           execute(merge_request)
@@ -59,10 +59,13 @@ module MergeRequests
     def reload_merge_requests
       merge_requests = @project.merge_requests.opened.
         by_source_or_target_branch(@branch_name).to_a
-      merge_requests += fork_merge_requests
-      merge_requests = filter_merge_requests(merge_requests)
 
-      merge_requests.each do |merge_request|
+      # Fork merge requests
+      merge_requests += MergeRequest.opened
+        .where(source_branch: @branch_name, source_project: @project)
+        .where.not(target_project: @project).to_a
+
+      filter_merge_requests(merge_requests).each do |merge_request|
         if merge_request.source_branch == @branch_name || force_push?
           merge_request.reload_diff
         else
@@ -76,9 +79,11 @@ module MergeRequests
       end
     end
 
-    # Reset approvals for merge request
+    # Note: Closed merge requests also need approvals reset.
     def reset_approvals_for_merge_requests
-      merge_requests_for_source_branch.each do |merge_request|
+      merge_requests = merge_requests_for(@branch_name, mr_states: [:opened, :reopened, :closed])
+
+      merge_requests.each do |merge_request|
         target_project = merge_request.target_project
 
         if target_project.approvals_before_merge.nonzero? &&
@@ -190,16 +195,7 @@ module MergeRequests
     end
 
     def merge_requests_for_source_branch
-      @source_merge_requests ||= begin
-        merge_requests = @project.origin_merge_requests.opened.where(source_branch: @branch_name).to_a
-        merge_requests += fork_merge_requests
-        filter_merge_requests(merge_requests)
-      end
-    end
-
-    def fork_merge_requests
-      @fork_merge_requests ||= @project.fork_merge_requests.opened.
-        where(source_branch: @branch_name).to_a
+      @source_merge_requests ||= merge_requests_for(@branch_name)
     end
 
     def branch_added?

changelogs/unreleased-ee/1457-reset-approvals-on-closed-mrs.yml

+---
+title: Also reset approvals on push when merge request is closed
+merge_request: 1051
+author:

spec/services/merge_requests/refresh_service_spec.rb

@@ -118,25 +118,50 @@ describe MergeRequests::RefreshService, services: true do
 
     context 'push to fork repo source branch' do
       let(:refresh_service) { service.new(@fork_project, @user) }
-      before do
-        allow(refresh_service).to receive(:execute_hooks)
-        refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
-        reload_mrs
-      end
 
-      it 'executes hooks with update action' do
-        expect(refresh_service).to have_received(:execute_hooks).
-          with(@fork_merge_request, 'update', @oldrev)
+      context 'open fork merge request' do
+        before do
+          allow(refresh_service).to receive(:execute_hooks)
+          refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+          reload_mrs
+        end
+
+        it 'executes hooks with update action' do
+          expect(refresh_service).to have_received(:execute_hooks).
+            with(@fork_merge_request, 'update', @oldrev)
+        end
+
+        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 28 commits') }
+        it { expect(@fork_merge_request).to be_open }
+        it { expect(@build_failed_todo).to be_pending }
+        it { expect(@fork_build_failed_todo).to be_pending }
+        it { expect(@fork_merge_request.approvals).to be_empty }
       end
 
-      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 28 commits') }
-      it { expect(@fork_merge_request).to be_open }
-      it { expect(@build_failed_todo).to be_pending }
-      it { expect(@fork_build_failed_todo).to be_pending }
-      it { expect(@fork_merge_request.approvals).to be_empty }
+      context 'closed fork merge request' do
+        before do
+          @fork_merge_request.close!
+          allow(refresh_service).to receive(:execute_hooks)
+          refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+          reload_mrs
+        end
+
+        it 'do not execute hooks with update action' do
+          expect(refresh_service).not_to have_received(:execute_hooks)
+        end
+
+        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_closed }
+        it { expect(@build_failed_todo).to be_pending }
+        it { expect(@fork_build_failed_todo).to be_pending }
+        it { expect(@fork_merge_request.approvals).to be_empty }
+      end
     end
 
     context 'push to fork repo target branch' do
@@ -197,7 +222,7 @@ describe MergeRequests::RefreshService, services: true do
         end
       end
 
-      context 'when approvals_before_merge is disabled' do
+      context 'when reset_approvals_on_push is disabled' do
         before do
           @project.update(reset_approvals_on_push: false)
           refresh_service = service.new(@project, @user)
@@ -225,16 +250,32 @@ describe MergeRequests::RefreshService, services: true do
         end
       end
 
-      context 'when there are approvals to be reset' do
-        before do
-          refresh_service = service.new(@project, @user)
-          allow(refresh_service).to receive(:execute_hooks)
-          refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
-          reload_mrs
+      context 'when there are approvals' do
+        context 'closed merge request' do
+          before do
+            @merge_request.close!
+            refresh_service = service.new(@project, @user)
+            allow(refresh_service).to receive(:execute_hooks)
+            refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+            reload_mrs
+          end
+
+          it 'resets the approvals' do
+            expect(@merge_request.approvals).to be_empty
+          end
         end
 
-        it 'resets the approvals' do
-          expect(@merge_request.approvals).to be_empty
+        context 'opened merge request' do
+          before do
+            refresh_service = service.new(@project, @user)
+            allow(refresh_service).to receive(:execute_hooks)
+            refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+            reload_mrs
+          end
+
+          it 'resets the approvals' do
+            expect(@merge_request.approvals).to be_empty
+          end
         end
       end
     end