Fix updating approvals count when editing an MR

app/controllers/projects/merge_requests_controller.rb

@@ -706,8 +706,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     target_project = @project.forked_from_project if @project.id.to_s != mr_params[:target_project_id]
     target_project ||= @project
 
+    # If the number of approvals is not greater than the project default, set to nil,
+    # so that we fall back to the project default.
     if mr_params[:approvals_before_merge].to_i <= target_project.approvals_before_merge
-      mr_params.delete(:approvals_before_merge)
+      mr_params[:approvals_before_merge] = nil
     end
 
     mr_params

changelogs/unreleased-ee/1591_approvals_update_fix.yml

+---
+title: Fix updating approvals count when editing an MR
+merge_request: 1106
+author:

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -400,42 +400,87 @@ describe Projects::MergeRequestsController do
     context 'the approvals_before_merge param' do
       before { project.update_attributes(approvals_before_merge: 2) }
 
-      context 'when it is less than the one in the target project' do
-        before { update_merge_request(approvals_before_merge: 1) }
+      context 'approvals_before_merge not set for the existing MR' do
+        context 'when it is less than the one in the target project' do
+          before { update_merge_request(approvals_before_merge: 1) }
 
-        it 'sets the param to nil' do
-          expect(merge_request.reload.approvals_before_merge).to eq(nil)
-        end
+          it 'sets the param to nil' do
+            expect(merge_request.reload.approvals_before_merge).to eq(nil)
+          end
 
-        it 'updates the merge request' do
-          expect(merge_request.reload).to be_valid
-          expect(response).to redirect_to(namespace_project_merge_request_path(id: merge_request.iid, project_id: project.to_param))
+          it 'updates the merge request' do
+            expect(merge_request.reload).to be_valid
+            expect(response).to redirect_to(namespace_project_merge_request_path(id: merge_request.iid, project_id: project.to_param))
+          end
         end
-      end
 
-      context 'when it is equal to the one in the target project' do
-        before { update_merge_request(approvals_before_merge: 2) }
+        context 'when it is equal to the one in the target project' do
+          before { update_merge_request(approvals_before_merge: 2) }
 
-        it 'sets the param to nil' do
-          expect(merge_request.reload.approvals_before_merge).to eq(nil)
+          it 'sets the param to nil' do
+            expect(merge_request.reload.approvals_before_merge).to eq(nil)
+          end
+
+          it 'updates the merge request' do
+            expect(merge_request.reload).to be_valid
+            expect(response).to redirect_to(namespace_project_merge_request_path(id: merge_request.iid, project_id: project.to_param))
+          end
         end
 
-        it 'updates the merge request' do
-          expect(merge_request.reload).to be_valid
-          expect(response).to redirect_to(namespace_project_merge_request_path(id: merge_request.iid, project_id: project.to_param))
+        context 'when it is greater than the one in the target project' do
+          before { update_merge_request(approvals_before_merge: 3) }
+
+          it 'saves the param in the merge request' do
+            expect(merge_request.reload.approvals_before_merge).to eq(3)
+          end
+
+          it 'updates the merge request' do
+            expect(merge_request.reload).to be_valid
+            expect(response).to redirect_to(namespace_project_merge_request_path(id: merge_request.iid, project_id: project.to_param))
+          end
         end
       end
 
-      context 'when it is greater than the one in the target project' do
-        before { update_merge_request(approvals_before_merge: 3) }
+      context 'approvals_before_merge set for the existing MR' do
+        before { merge_request.update_attribute(:approvals_before_merge, 4) }
 
-        it 'saves the param in the merge request' do
-          expect(merge_request.reload.approvals_before_merge).to eq(3)
+        context 'when it is less than the one in the target project' do
+          before { update_merge_request(approvals_before_merge: 1) }
+
+          it 'sets the param to nil' do
+            expect(merge_request.reload.approvals_before_merge).to eq(nil)
+          end
+
+          it 'updates the merge request' do
+            expect(merge_request.reload).to be_valid
+            expect(response).to redirect_to(namespace_project_merge_request_path(id: merge_request.iid, project_id: project.to_param))
+          end
         end
 
-        it 'updates the merge request' do
-          expect(merge_request.reload).to be_valid
-          expect(response).to redirect_to(namespace_project_merge_request_path(id: merge_request.iid, project_id: project.to_param))
+        context 'when it is equal to the one in the target project' do
+          before { update_merge_request(approvals_before_merge: 2) }
+
+          it 'sets the param to nil' do
+            expect(merge_request.reload.approvals_before_merge).to eq(nil)
+          end
+
+          it 'updates the merge request' do
+            expect(merge_request.reload).to be_valid
+            expect(response).to redirect_to(namespace_project_merge_request_path(id: merge_request.iid, project_id: project.to_param))
+          end
+        end
+
+        context 'when it is greater than the one in the target project' do
+          before { update_merge_request(approvals_before_merge: 3) }
+
+          it 'saves the param in the merge request' do
+            expect(merge_request.reload.approvals_before_merge).to eq(3)
+          end
+
+          it 'updates the merge request' do
+            expect(merge_request.reload).to be_valid
+            expect(response).to redirect_to(namespace_project_merge_request_path(id: merge_request.iid, project_id: project.to_param))
+          end
         end
       end
     end