Merge branch 'fix-suggested-approvers-on-mr-update' into 'master'

Fix suggested approvers on MR update

This was three issues combining:

1. https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/470 didn't apply the same coercion of the `approvals_before_merge` param to editing an existing MR as there was for creating a new MR. This meant that creating a new MR with the same number of approvers as the target project, editing it, not changing anything, and clicking Save would produce a validation error.
2. `set_suggested_approvers` wasn't called in a couple of places where we render the MR edit form (like the above).
3. The number of approvals used for clamping the `approvals_before_merge` param was based on the _source_ project, not the _target_ project (unlike the model validation).

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/18946, closes https://gitlab.com/gitlab-org/gitlab-ee/issues/710, and closes https://gitlab.com/gitlab-org/gitlab-ee/issues/711.

See merge request !496

CHANGELOG-EE

@@ -2,6 +2,10 @@ Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.10.0 (unreleased)
 
+v 8.9.1
+  - Fix MR creation from forks where target project has approvals enabled
+  - Fix MR edit where target project has approvals enabled
+
 v 8.9.0
   - Fix JenkinsService test button
   - Fix nil user handling in UpdateMirrorService

app/controllers/projects/merge_requests_controller.rb

@@ -136,11 +136,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def create
     @target_branches ||= []
-    create_params = merge_request_params
-
-    if create_params[:approvals_before_merge].to_i <= project.approvals_before_merge
-      create_params.delete(:approvals_before_merge)
-    end
+    create_params = clamp_approvals_before_merge(merge_request_params)
 
     @merge_request = MergeRequests::CreateService.new(project, current_user, create_params).execute
 
@@ -149,6 +145,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     else
       @source_project = @merge_request.source_project
       @target_project = @merge_request.target_project
+      set_suggested_approvers
+
       render action: "new"
     end
   end
@@ -162,7 +160,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def update
-    @merge_request = MergeRequests::UpdateService.new(project, current_user, merge_request_params).execute(@merge_request)
+    update_params = clamp_approvals_before_merge(merge_request_params)
+
+    @merge_request = MergeRequests::UpdateService.new(project, current_user, update_params).execute(@merge_request)
 
     if @merge_request.valid?
       respond_to do |format|
@@ -175,6 +175,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
         end
       end
     else
+      set_suggested_approvers
+
       render "edit"
     end
   end
@@ -407,6 +409,14 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     )
   end
 
+  def clamp_approvals_before_merge(mr_params)
+    if mr_params[:approvals_before_merge].to_i <= selected_target_project.approvals_before_merge
+      mr_params.delete(:approvals_before_merge)
+    end
+
+    mr_params
+  end
+
   def merge_params
     params.permit(:should_remove_source_branch, :commit_message)
   end

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -222,6 +222,14 @@ describe Projects::MergeRequestsController do
   end
 
   describe 'PUT #update' do
+    def update_merge_request(params = {})
+      post :update,
+           namespace_id: project.namespace.to_param,
+           project_id: project.to_param,
+           id: merge_request.iid,
+           merge_request: params
+    end
+
     context 'there is no source project' do
       let(:project)       { create(:project) }
       let(:fork_project)  { create(:forked_project_with_submodules) }
@@ -235,18 +243,55 @@ describe Projects::MergeRequestsController do
       end
 
       it 'closes MR without errors' do
-        post :update,
-            namespace_id: project.namespace.path,
-            project_id: project.path,
-            id: merge_request.iid,
-            merge_request: {
-              state_event: 'close'
-            }
+        update_merge_request(state_event: 'close')
 
         expect(response).to redirect_to([merge_request.target_project.namespace.becomes(Namespace), merge_request.target_project, merge_request])
         expect(merge_request.reload.closed?).to be_truthy
       end
     end
+
+    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) }
+
+        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 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
 
   describe 'POST #merge' do

spec/features/merge_requests/create_new_mr_spec.rb

@@ -40,13 +40,24 @@ feature 'Create New Merge Request', feature: true, js: true do
   end
 
   context 'when approvals are enabled for the target project' do
-    before { project.update_attributes(approvals_before_merge: 1) }
+    before do
+      project.update_attributes(approvals_before_merge: 1)
 
-    it 'shows approval settings' do
       visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { source_branch: 'feature_conflict' })
+    end
 
+    it 'shows approval settings' do
       expect(page).to have_content('Approvers')
     end
+
+    context 'saving the MR' do
+      it 'shows the saved MR' do
+        fill_in 'merge_request_title', with: 'Test'
+        click_button 'Submit merge request'
+
+        expect(page).to have_link('Close merge request')
+      end
+    end
   end
 
   context 'when target project cannot be viewed by the current user' do

spec/features/merge_requests/edit_mr_spec.rb

@@ -7,15 +7,24 @@ feature 'Edit Merge Request', feature: true do
 
   before do
     project.team << [user, :master]
+    project.update_attributes(approvals_before_merge: 2)
 
     login_as user
 
     visit edit_namespace_project_merge_request_path(project.namespace, project, merge_request)
   end
 
-  context 'editing a MR' do
+  context 'editing a MR that needs approvals' do
     it 'form should have class js-quick-submit' do
       expect(page).to have_selector('.js-quick-submit')
     end
+
+    context 'saving the MR' do
+      it 'shows the saved MR' do
+        click_button 'Save changes'
+
+        expect(page).to have_link('Close merge request')
+      end
+    end
   end
 end