Reset closed merge request approvals on "push to reset approvals"

Also, we've fixed a bug on Project#fork_merge_requests, where it was also
including MR's sent to itself. Beside that, we might be improving the
performance when fetching the origin and fork merge requests by
submitting just two queries (including the `source_project` preload).

app/models/project.rb

@@ -120,8 +120,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

@@ -42,7 +42,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)
@@ -58,10 +58,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
@@ -75,9 +78,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? &&
@@ -171,16 +176,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