Refactor Projects::MergeRequestsController into separate CE and EE files

app/controllers/ee/projects/merge_requests_controller.rb

+module EE
+  module Projects
+    module MergeRequestsController
+      extend ActiveSupport::Concern
+
+      prepended do
+        # This module is prepended to `Projects::MergeRequstController`, which
+        # already calls `before_action :merge_request, only: [...]`. Calling it
+        # again here would *replace* the restriction, rather than extending it.
+        before_action(only: [:approve, :approvals, :unapprove, :rebase]) { merge_request }
+
+        before_action :set_suggested_approvers, only: [:new, :new_diffs, :edit]
+      end
+
+      def rebase
+        return access_denied! unless @merge_request.can_be_merged_by?(current_user)
+        return render_404 unless @merge_request.approved?
+
+        RebaseWorker.perform_async(@merge_request.id, current_user.id)
+
+        render nothing: true, status: 200
+      end
+
+      def approve
+        unless @merge_request.can_approve?(current_user)
+          return render_404
+        end
+
+        ::MergeRequests::ApprovalService
+          .new(project, current_user)
+          .execute(@merge_request)
+
+        render_approvals_json
+      end
+
+      def approvals
+        render_approvals_json
+      end
+
+      def unapprove
+        if @merge_request.has_approved?(current_user)
+          ::MergeRequests::RemoveApprovalService
+            .new(project, current_user)
+            .execute(@merge_request)
+        end
+
+        render_approvals_json
+      end
+
+      protected
+
+      def render_approvals_json
+        respond_to do |format|
+          format.json do
+            entity = ::API::Entities::MergeRequestApprovals.new(@merge_request, current_user: current_user)
+            render json: entity
+          end
+        end
+      end
+
+      def set_suggested_approvers
+        if @merge_request.requires_approve?
+          @suggested_approvers = ::Gitlab::AuthorityAnalyzer.new(
+            @merge_request,
+            @merge_request.author || current_user
+          ).calculate(@merge_request.approvals_required)
+        end
+      end
+
+      def merge_params_attributes
+        super + [:squash]
+      end
+
+      def merge_request_params
+        clamp_approvals_before_merge(super)
+      end
+
+      def merge_request_params_attributes
+        super + %i[
+          approvals_before_merge
+          approver_group_ids
+          approver_ids
+          squash
+        ]
+      end
+
+      # 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 it's not set, we can
+      # let the normal update logic handle this.
+      def clamp_approvals_before_merge(mr_params)
+        return mr_params unless mr_params[:approvals_before_merge]
+
+        target_project = @project.forked_from_project if @project.id.to_s != mr_params[:target_project_id]
+        target_project ||= @project
+
+        if mr_params[:approvals_before_merge].to_i <= target_project.approvals_before_merge
+          mr_params[:approvals_before_merge] = nil
+        end
+
+        mr_params
+      end
+    end
+  end
+end

app/controllers/projects/merge_requests_controller.rb

@@ -10,10 +10,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   before_action :check_merge_requests_available!
   before_action :merge_request, only: [
     :edit, :update, :show, :diffs, :commits, :conflicts, :conflict_for_path, :pipelines, :merge,
-    :pipeline_status, :ci_environments_status, :toggle_subscription, :cancel_merge_when_pipeline_succeeds,
-    :remove_wip, :resolve_conflicts, :assign_related_issues, :commit_change_content,
-    # EE
-    :approve, :approvals, :unapprove, :rebase
+    :pipeline_status, :ci_environments_status, :toggle_subscription, :cancel_merge_when_pipeline_succeeds, :remove_wip, :resolve_conflicts, :assign_related_issues, :commit_change_content
   ]
   before_action :validates_merge_request, only: [:show, :diffs, :commits, :pipelines]
   before_action :define_show_vars, only: [:diffs, :commits, :conflicts, :conflict_for_path, :builds, :pipelines]
@@ -22,7 +19,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   before_action :check_if_can_be_merged, only: :show
   before_action :apply_diff_view_cookie!, only: [:new_diffs]
   before_action :build_merge_request, only: [:new, :new_diffs]
-  before_action :set_suggested_approvers, only: [:new, :new_diffs, :edit]
 
   # Allow read any merge_request
   before_action :authorize_read_merge_request!
@@ -37,6 +33,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   before_action :authorize_can_resolve_conflicts!, only: [:conflicts, :conflict_for_path, :resolve_conflicts]
 
+  # This module must be prepended *after* all before_action filters
+  prepend ::EE::Projects::MergeRequestsController
+
   def index
     @collection_type    = "MergeRequest"
     @merge_requests     = merge_requests_collection
@@ -269,9 +268,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def create
     @target_branches ||= []
-    create_params = clamp_approvals_before_merge(merge_request_params)
-
-    @merge_request = MergeRequests::CreateService.new(project, current_user, create_params).execute
+    @merge_request = MergeRequests::CreateService.new(project, current_user, merge_request_params).execute
 
     if @merge_request.valid?
       redirect_to(merge_request_path(@merge_request))
@@ -291,9 +288,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def update
-    update_params = clamp_approvals_before_merge(merge_request_params)
-
-    @merge_request = MergeRequests::UpdateService.new(project, current_user, update_params).execute(@merge_request)
+    @merge_request = MergeRequests::UpdateService.new(project, current_user, merge_request_params).execute(@merge_request)
 
     respond_to do |format|
       format.html do
@@ -340,15 +335,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     render json: serializer.represent(@merge_request)
   end
 
-  def rebase
-    return access_denied! unless @merge_request.can_be_merged_by?(current_user)
-    return render_404 unless @merge_request.approved?
-
-    RebaseWorker.perform_async(@merge_request.id, current_user.id)
-
-    render nothing: true, status: 200
-  end
-
   def merge
     return access_denied! unless @merge_request.can_be_merged_by?(current_user)
     return render_404 unless @merge_request.approved?
@@ -432,9 +418,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
           metrics_url =
             if can?(current_user, :read_environment, environment) && environment.has_metrics?
               metrics_namespace_project_environment_deployment_path(environment.project.namespace,
-                                                         environment.project,
-                                                         environment,
-                                                         deployment)
+                                                                    environment.project,
+                                                                    environment,
+                                                                    deployment)
             end
 
           {
@@ -454,43 +440,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     render json: environments
   end
 
-  def approve
-    unless @merge_request.can_approve?(current_user)
-      return render_404
-    end
-
-    ::MergeRequests::ApprovalService
-      .new(project, current_user)
-      .execute(@merge_request)
-
-    render_approvals_json
-  end
-
-  def approvals
-    render_approvals_json
-  end
-
-  def unapprove
-    if @merge_request.has_approved?(current_user)
-      ::MergeRequests::RemoveApprovalService
-        .new(project, current_user)
-        .execute(@merge_request)
-    end
-
-    render_approvals_json
-  end
-
   protected
 
-  def render_approvals_json
-    respond_to do |format|
-      format.json do
-        entity = API::Entities::MergeRequestApprovals.new(@merge_request, current_user: current_user)
-        render json: entity
-      end
-    end
-  end
-
   def selected_target_project
     if @project.id.to_s == params[:target_project_id] || @project.forked_project_link.nil?
       @project
@@ -632,15 +583,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     render 'invalid'
   end
 
-  def set_suggested_approvers
-    if @merge_request.requires_approve?
-      @suggested_approvers = Gitlab::AuthorityAnalyzer.new(
-        @merge_request,
-        @merge_request.author || current_user
-      ).calculate(@merge_request.approvals_required)
-    end
-  end
-
   def merge_request_params
     params.require(:merge_request)
       .permit(merge_request_params_attributes)
@@ -662,36 +604,15 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       :title,
 
       label_ids: []
-    ] + merge_request_params_ee
-  end
-
-  def merge_request_params_ee
-    %i[
-      approvals_before_merge
-      approver_group_ids
-      approver_ids
-      squash
     ]
   end
 
-  # 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 it's not set, we can
-  # let the normal update logic handle this.
-  def clamp_approvals_before_merge(mr_params)
-    return mr_params unless mr_params[:approvals_before_merge]
-
-    target_project = @project.forked_from_project if @project.id.to_s != mr_params[:target_project_id]
-    target_project ||= @project
-
-    if mr_params[:approvals_before_merge].to_i <= target_project.approvals_before_merge
-      mr_params[:approvals_before_merge] = nil
-    end
-
-    mr_params
+  def merge_params
+    params.permit(merge_params_attributes)
   end
 
-  def merge_params
-    params.permit(:should_remove_source_branch, :commit_message, :squash)
+  def merge_params_attributes
+    [:should_remove_source_branch, :commit_message]
   end
 
   # Make sure merge requests created before 8.0

spec/controllers/projects/merge_requests_controller_ee_spec.rb

+require 'spec_helper'
+
+describe Projects::MergeRequestsController do
+  let(:project) { create(:project) }
+  let(:user)    { project.owner }
+  let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
+
+  before do
+    sign_in(user)
+  end
+
+  describe 'POST #create' do
+    def create_merge_request(overrides = {})
+      params = {
+        namespace_id: project.namespace.to_param,
+        project_id: project.to_param,
+        merge_request: {
+          title: 'Test',
+          source_branch: 'feature_conflict',
+          target_branch: 'master',
+          author: user
+        }.merge(overrides)
+      }
+
+      post :create, params
+    end
+
+    context 'the approvals_before_merge param' do
+      let(:created_merge_request) { assigns(:merge_request) }
+
+      before do
+        project.update_attributes(approvals_before_merge: 2)
+      end
+
+      context 'when it is less than the one in the target project' do
+        before do
+          create_merge_request(approvals_before_merge: 1)
+        end
+
+        it 'sets the param to nil' do
+          expect(created_merge_request.approvals_before_merge).to eq(nil)
+        end
+
+        it 'creates the merge request' do
+          expect(created_merge_request).to be_valid
+          expect(response).to redirect_to(namespace_project_merge_request_path(id: created_merge_request.iid, project_id: project.to_param))
+        end
+      end
+
+      context 'when it is equal to the one in the target project' do
+        before do
+          create_merge_request(approvals_before_merge: 2)
+        end
+
+        it 'sets the param to nil' do
+          expect(created_merge_request.approvals_before_merge).to eq(nil)
+        end
+
+        it 'creates the merge request' do
+          expect(created_merge_request).to be_valid
+          expect(response).to redirect_to(namespace_project_merge_request_path(id: created_merge_request.iid, project_id: project.to_param))
+        end
+      end
+
+      context 'when it is greater than the one in the target project' do
+        before do
+          create_merge_request(approvals_before_merge: 3)
+        end
+
+        it 'saves the param in the merge request' do
+          expect(created_merge_request.approvals_before_merge).to eq(3)
+        end
+
+        it 'creates the merge request' do
+          expect(created_merge_request).to be_valid
+          expect(response).to redirect_to(namespace_project_merge_request_path(id: created_merge_request.iid, project_id: project.to_param))
+        end
+      end
+
+      context 'when the target project is a fork of a deleted project' do
+        before do
+          original_project = create(:empty_project)
+          project.update_attributes(forked_from_project: original_project, approvals_before_merge: 4)
+          original_project.update_attributes(pending_delete: true)
+
+          create_merge_request(approvals_before_merge: 3)
+        end
+
+        it 'uses the default from the target project' do
+          expect(created_merge_request.approvals_before_merge).to eq(nil)
+        end
+
+        it 'creates the merge request' do
+          expect(created_merge_request).to be_valid
+          expect(response).to redirect_to(namespace_project_merge_request_path(id: created_merge_request.iid, project_id: project.to_param))
+        end
+      end
+    end
+  end
+
+  context 'approvals' do
+    def json_response
+      JSON.parse(response.body)
+    end
+
+    let(:approver) { create(:user) }
+
+    before do
+      merge_request.update_attribute :approvals_before_merge, 2
+      project.team << [approver, :developer]
+      project.approver_ids = [user, approver].map(&:id).join(',')
+    end
+
+    describe 'approve' do
+      before do
+        post :approve,
+          namespace_id: project.namespace.to_param,
+          project_id: project.to_param,
+          id: merge_request.iid,
+          format: :json
+      end
+
+      it 'approves the merge request' do
+        expect(response).to be_success
+        expect(json_response['approvals_left']).to eq 1
+        expect(json_response['approved_by'].size).to eq 1
+        expect(json_response['approved_by'][0]['user']['username']).to eq user.username
+        expect(json_response['user_has_approved']).to be true
+        expect(json_response['user_can_approve']).to be false
+        expect(json_response['suggested_approvers'].size).to eq 1
+        expect(json_response['suggested_approvers'][0]['username']).to eq approver.username
+      end
+    end
+
+    describe 'approvals' do
+      before do
+        merge_request.approvals.create(user: approver)
+        get :approvals,
+          namespace_id: project.namespace.to_param,
+          project_id: project.to_param,
+          id: merge_request.iid,
+          format: :json
+      end
+
+      it 'shows approval information' do
+        expect(response).to be_success
+        expect(json_response['approvals_left']).to eq 1
+        expect(json_response['approved_by'].size).to eq 1
+        expect(json_response['approved_by'][0]['user']['username']).to eq approver.username
+        expect(json_response['user_has_approved']).to be false
+        expect(json_response['user_can_approve']).to be true
+        expect(json_response['suggested_approvers'].size).to eq 1
+        expect(json_response['suggested_approvers'][0]['username']).to eq user.username
+      end
+    end
+
+    describe 'unapprove' do
+      before do
+        merge_request.approvals.create(user: user)
+        delete :unapprove,
+          namespace_id: project.namespace.to_param,
+          project_id: project.to_param,
+          id: merge_request.iid,
+          format: :json
+      end
+
+      it 'unapproves the merge request' do
+        expect(response).to be_success
+        expect(json_response['approvals_left']).to eq 2
+        expect(json_response['approved_by']).to be_empty
+        expect(json_response['user_has_approved']).to be false
+        expect(json_response['user_can_approve']).to be true
+        expect(json_response['suggested_approvers'].size).to eq 2
+      end
+    end
+  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 'when the merge request requires approval' do
+      before do
+        project.update_attributes(approvals_before_merge: 1)
+      end
+
+      it_behaves_like 'update invalid issuable', MergeRequest
+    end
+
+    context 'the approvals_before_merge param' do
+      before do
+        project.update_attributes(approvals_before_merge: 2)
+      end
+
+      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 do
+            update_merge_request(approvals_before_merge: 1)
+          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))
+          end
+        end
+
+        context 'when it is equal to the one in the target project' do
+          before do
+            update_merge_request(approvals_before_merge: 2)
+          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))
+          end
+        end
+
+        context 'when it is greater than the one in the target project' do
+          before do
+            update_merge_request(approvals_before_merge: 3)
+          end
+
+          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 'approvals_before_merge set for the existing MR' do
+        before do
+          merge_request.update_attribute(:approvals_before_merge, 4)
+        end
+
+        context 'when it is not set' do
+          before do
+            update_merge_request(title: 'New title')
+          end
+
+          it 'does not change the merge request' do
+            expect(merge_request.reload.approvals_before_merge).to eq(4)
+          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 less than the one in the target project' do
+          before do
+            update_merge_request(approvals_before_merge: 1)
+          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))
+          end
+        end
+
+        context 'when it is equal to the one in the target project' do
+          before do
+            update_merge_request(approvals_before_merge: 2)
+          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))
+          end
+        end
+
+        context 'when it is greater than the one in the target project' do
+          before do
+            update_merge_request(approvals_before_merge: 3)
+          end
+
+          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
+  end
+
+  describe 'POST merge' do
+    let(:base_params) do
+      {
+        namespace_id: project.namespace,
+        project_id: project,
+        id: merge_request.iid,
+        squash: false,
+        format: 'json'
+      }
+    end
+
+    context 'when the sha parameter matches the source SHA' do
+      def merge_with_sha(params = {})
+        post :merge, base_params.merge(sha: merge_request.diff_head_sha).merge(params)
+      end
+
+      context 'when squash is passed as 1' do
+        it 'updates the squash attribute on the MR to true' do
+          merge_request.update(squash: false)
+          merge_with_sha(squash: '1')
+
+          expect(merge_request.reload.squash).to be_truthy
+        end
+      end
+
+      context 'when squash is passed as 0' do
+        it 'updates the squash attribute on the MR to false' do
+          merge_request.update(squash: true)
+          merge_with_sha(squash: '0')
+
+          expect(merge_request.reload.squash).to be_falsey
+        end
+      end
+    end
+  end
+end

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -61,178 +61,6 @@ describe Projects::MergeRequestsController do
     end
   end
 
-  describe 'POST #create' do
-    def create_merge_request(overrides = {})
-      params = {
-        namespace_id: project.namespace.to_param,
-        project_id: project.to_param,
-        merge_request: {
-          title: 'Test',
-          source_branch: 'feature_conflict',
-          target_branch: 'master',
-          author: user
-        }.merge(overrides)
-      }
-
-      post :create, params
-    end
-
-    context 'the approvals_before_merge param' do
-      let(:created_merge_request) { assigns(:merge_request) }
-
-      before do
-        project.update_attributes(approvals_before_merge: 2)
-      end
-
-      context 'when it is less than the one in the target project' do
-        before do
-          create_merge_request(approvals_before_merge: 1)
-        end
-
-        it 'sets the param to nil' do
-          expect(created_merge_request.approvals_before_merge).to eq(nil)
-        end
-
-        it 'creates the merge request' do
-          expect(created_merge_request).to be_valid
-          expect(response).to redirect_to(namespace_project_merge_request_path(id: created_merge_request.iid, project_id: project.to_param))
-        end
-      end
-
-      context 'when it is equal to the one in the target project' do
-        before do
-          create_merge_request(approvals_before_merge: 2)
-        end
-
-        it 'sets the param to nil' do
-          expect(created_merge_request.approvals_before_merge).to eq(nil)
-        end
-
-        it 'creates the merge request' do
-          expect(created_merge_request).to be_valid
-          expect(response).to redirect_to(namespace_project_merge_request_path(id: created_merge_request.iid, project_id: project.to_param))
-        end
-      end
-
-      context 'when it is greater than the one in the target project' do
-        before do
-          create_merge_request(approvals_before_merge: 3)
-        end
-
-        it 'saves the param in the merge request' do
-          expect(created_merge_request.approvals_before_merge).to eq(3)
-        end
-
-        it 'creates the merge request' do
-          expect(created_merge_request).to be_valid
-          expect(response).to redirect_to(namespace_project_merge_request_path(id: created_merge_request.iid, project_id: project.to_param))
-        end
-      end
-
-      context 'when the target project is a fork of a deleted project' do
-        before do
-          original_project = create(:empty_project)
-          project.update_attributes(forked_from_project: original_project, approvals_before_merge: 4)
-          original_project.update_attributes(pending_delete: true)
-
-          create_merge_request(approvals_before_merge: 3)
-        end
-
-        it 'uses the default from the target project' do
-          expect(created_merge_request.approvals_before_merge).to eq(nil)
-        end
-
-        it 'creates the merge request' do
-          expect(created_merge_request).to be_valid
-          expect(response).to redirect_to(namespace_project_merge_request_path(id: created_merge_request.iid, project_id: project.to_param))
-        end
-      end
-    end
-
-    context 'when the merge request is invalid' do
-      it 'shows the #new form' do
-        expect(create_merge_request(title: nil)).to render_template(:new)
-      end
-    end
-  end
-
-  context 'approvals' do
-    def json_response
-      JSON.parse(response.body)
-    end
-
-    let(:approver) { create(:user) }
-
-    before do
-      merge_request.update_attribute :approvals_before_merge, 2
-      project.team << [approver, :developer]
-      project.approver_ids = [user, approver].map(&:id).join(',')
-    end
-
-    describe 'approve' do
-      before do
-        post :approve,
-          namespace_id: project.namespace.to_param,
-          project_id: project.to_param,
-          id: merge_request.iid,
-          format: :json
-      end
-
-      it 'approves the merge request' do
-        expect(response).to be_success
-        expect(json_response['approvals_left']).to eq 1
-        expect(json_response['approved_by'].size).to eq 1
-        expect(json_response['approved_by'][0]['user']['username']).to eq user.username
-        expect(json_response['user_has_approved']).to be true
-        expect(json_response['user_can_approve']).to be false
-        expect(json_response['suggested_approvers'].size).to eq 1
-        expect(json_response['suggested_approvers'][0]['username']).to eq approver.username
-      end
-    end
-
-    describe 'approvals' do
-      before do
-        merge_request.approvals.create(user: approver)
-        get :approvals,
-          namespace_id: project.namespace.to_param,
-          project_id: project.to_param,
-          id: merge_request.iid,
-          format: :json
-      end
-
-      it 'shows approval information' do
-        expect(response).to be_success
-        expect(json_response['approvals_left']).to eq 1
-        expect(json_response['approved_by'].size).to eq 1
-        expect(json_response['approved_by'][0]['user']['username']).to eq approver.username
-        expect(json_response['user_has_approved']).to be false
-        expect(json_response['user_can_approve']).to be true
-        expect(json_response['suggested_approvers'].size).to eq 1
-        expect(json_response['suggested_approvers'][0]['username']).to eq user.username
-      end
-    end
-
-    describe 'unapprove' do
-      before do
-        merge_request.approvals.create(user: user)
-        delete :unapprove,
-          namespace_id: project.namespace.to_param,
-          project_id: project.to_param,
-          id: merge_request.iid,
-          format: :json
-      end
-
-      it 'unapproves the merge request' do
-        expect(response).to be_success
-        expect(json_response['approvals_left']).to eq 2
-        expect(json_response['approved_by']).to be_empty
-        expect(json_response['user_has_approved']).to be false
-        expect(json_response['user_can_approve']).to be true
-        expect(json_response['suggested_approvers'].size).to eq 2
-      end
-    end
-  end
-
   describe 'GET commit_change_content' do
     it 'renders commit_change_content template' do
       get :commit_change_content,
@@ -391,14 +219,6 @@ 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 'changing the assignee' do
       it 'limits the attributes exposed on the assignee' do
         assignee = create(:user)
@@ -430,7 +250,13 @@ describe Projects::MergeRequestsController do
       end
 
       it 'closes MR without errors' do
-        update_merge_request(state_event: 'close')
+        post :update,
+            namespace_id: project.namespace,
+            project_id: project,
+            id: merge_request.iid,
+            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
@@ -467,133 +293,6 @@ describe Projects::MergeRequestsController do
 
       it_behaves_like 'update invalid issuable', MergeRequest
     end
-
-    context 'when the merge request requires approval' do
-      before do
-        project.update_attributes(approvals_before_merge: 1)
-      end
-
-      it_behaves_like 'update invalid issuable', MergeRequest
-    end
-
-    context 'the approvals_before_merge param' do
-      before do
-        project.update_attributes(approvals_before_merge: 2)
-      end
-
-      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 do
-            update_merge_request(approvals_before_merge: 1)
-          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))
-          end
-        end
-
-        context 'when it is equal to the one in the target project' do
-          before do
-            update_merge_request(approvals_before_merge: 2)
-          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))
-          end
-        end
-
-        context 'when it is greater than the one in the target project' do
-          before do
-            update_merge_request(approvals_before_merge: 3)
-          end
-
-          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 'approvals_before_merge set for the existing MR' do
-        before do
-          merge_request.update_attribute(:approvals_before_merge, 4)
-        end
-
-        context 'when it is not set' do
-          before do
-            update_merge_request(title: 'New title')
-          end
-
-          it 'does not change the merge request' do
-            expect(merge_request.reload.approvals_before_merge).to eq(4)
-          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 less than the one in the target project' do
-          before do
-            update_merge_request(approvals_before_merge: 1)
-          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))
-          end
-        end
-
-        context 'when it is equal to the one in the target project' do
-          before do
-            update_merge_request(approvals_before_merge: 2)
-          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))
-          end
-        end
-
-        context 'when it is greater than the one in the target project' do
-          before do
-            update_merge_request(approvals_before_merge: 3)
-          end
-
-          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
   end
 
   describe 'POST merge' do
@@ -643,26 +342,8 @@ describe Projects::MergeRequestsController do
     end
 
     context 'when the sha parameter matches the source SHA' do
-      def merge_with_sha(params = {})
-        post :merge, base_params.merge(sha: merge_request.diff_head_sha).merge(params)
-      end
-
-      context 'when squash is passed as 1' do
-        it 'updates the squash attribute on the MR to true' do
-          merge_request.update(squash: false)
-          merge_with_sha(squash: '1')
-
-          expect(merge_request.reload.squash).to be_truthy
-        end
-      end
-
-      context 'when squash is passed as 1' do
-        it 'updates the squash attribute on the MR to false' do
-          merge_request.update(squash: true)
-          merge_with_sha(squash: '0')
-
-          expect(merge_request.reload.squash).to be_falsey
-        end
+      def merge_with_sha
+        post :merge, base_params.merge(sha: merge_request.diff_head_sha)
       end
 
       it 'returns :success' do