Merge branch '10055-approvals-required-max' into 'master'

Use max of approvals_before_merge between project and MR at runtime

Closes #10909 and #10055

See merge request gitlab-org/gitlab-ee!10766

ee/app/models/concerns/approvable.rb

@@ -46,7 +46,7 @@ module Approvable
   end
 
   def approvals_required
-    approvals_before_merge || target_project.approvals_before_merge
+    [approvals_before_merge.to_i, target_project.approvals_before_merge.to_i].max
   end
 
   def approvals_before_merge

ee/app/models/ee/merge_request.rb

@@ -22,7 +22,6 @@ module EE
       has_many :approval_rules, class_name: 'ApprovalMergeRequestRule', inverse_of: :merge_request
       has_many :draft_notes
 
-      validate :validate_approvals_before_merge, unless: :importing?
       validate :validate_approval_rule_source
 
       delegate :sha, to: :head_pipeline, prefix: :head_pipeline, allow_nil: true
@@ -69,19 +68,6 @@ module EE
       actual_head_pipeline&.latest_merge_request_pipeline?
     end
 
-    def validate_approvals_before_merge
-      return true unless approvals_before_merge
-      return true unless target_project
-
-      # Ensure per-merge-request approvals override is valid
-      if approvals_before_merge >= target_project.approvals_before_merge
-        true
-      else
-        errors.add :validate_approvals_before_merge,
-                   'Number of approvals must be at least that of approvals on the target project'
-      end
-    end
-
     def validate_approval_rule_source
       return if ::Feature.disabled?(:approval_rules, project, default_enabled: true)
       return unless approval_rules.any?

ee/changelogs/unreleased/10055-approvals-required-max.yml

+---
+title: Fix merge request operation failure (e.g. assigning user) when project approvers required increases
+merge_request: 10766
+author:
+type: fixed

ee/spec/models/merge_request_spec.rb

@@ -841,27 +841,27 @@ describe MergeRequest do
     end
   end
 
-  describe "#approvals_required" do
-    let(:merge_request) { build(:merge_request) }
-
-    before do
-      merge_request.target_project.update(approvals_before_merge: 3)
+  describe '#approvals_required' do
+    where(:license_value, :db_value, :project_db_value, :expected) do
+      true  | 5   | 6   | 6
+      true  | 6   | 5   | 6
+      true  | nil | 5   | 5
+      false | 5   | 6   | 0
+      false | nil | 5   | 0
     end
 
-    context "when the MR has approvals_before_merge set" do
+    with_them do
+      let(:merge_request) { build(:merge_request, approvals_before_merge: db_value) }
+
+      subject { merge_request.approvals_required }
+
       before do
-        merge_request.update(approvals_before_merge: 1)
-      end
+        stub_licensed_features(merge_request_approvers: license_value)
 
-      it "uses the approvals_before_merge from the MR" do
-        expect(merge_request.approvals_required).to eq(1)
+        merge_request.target_project.approvals_before_merge = project_db_value
       end
-    end
 
-    context "when the MR doesn't have approvals_before_merge set" do
-      it "takes approvals_before_merge from the target project" do
-        expect(merge_request.approvals_required).to eq(3)
-      end
+      it { is_expected.to eq(expected) }
     end
   end
 

ee/spec/requests/api/merge_request_approvals_spec.rb

@@ -82,7 +82,7 @@ describe API::MergeRequestApprovals do
           project.update_attribute(:disable_overriding_approvers_per_merge_request, false)
         end
 
-        it 'allows you to override approvals required' do
+        it 'allows you to set approvals_before_merge' do
           expect do
             post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), params: { approvals_required: 5 }
           end.to change { merge_request.reload.approvals_before_merge }.from(nil).to(5)
@@ -90,27 +90,6 @@ describe API::MergeRequestApprovals do
           expect(response).to have_gitlab_http_status(201)
           expect(json_response['approvals_required']).to eq(5)
         end
-
-        context 'when project approvals are zero' do
-          before do
-            project.update!(approvals_before_merge: 0)
-          end
-
-          it 'does not include an error in the response' do
-            expect do
-              post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), params: { approvals_required: 0 }
-            end.to change {merge_request.reload.approvals_before_merge}.from(nil).to(0)
-            expect(json_response['message']).to eq(nil)
-          end
-        end
-
-        it 'does not allow approvals required under what the project requires' do
-          expect do
-            post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), params: { approvals_required: 1 }
-          end.not_to change { merge_request.reload.approvals_before_merge }
-
-          expect(response).to have_gitlab_http_status(400)
-        end
       end
 
       context 'when disable_overriding_approvers_per_merge_request is true on the project' do
@@ -118,7 +97,7 @@ describe API::MergeRequestApprovals do
           project.update_attribute(:disable_overriding_approvers_per_merge_request, true)
         end
 
-        it 'does not allow you to override approvals required' do
+        it 'does not allow you to set approvals_before_merge' do
           expect do
             post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), params: { approvals_required: 5 }
           end.not_to change { merge_request.reload.approvals_before_merge }
@@ -544,13 +523,13 @@ describe "API::MergeRequestApprovals with approval_rule enabled" do
   end
 
   describe 'POST :id/merge_requests/:merge_request_iid/approvals' do
-    shared_examples_for 'user allowed to override approvals required' do
+    shared_examples_for 'user allowed to override approvals_before_merge' do
       context 'when disable_overriding_approvers_per_merge_request is false on the project' do
         before do
           project.update(disable_overriding_approvers_per_merge_request: false)
         end
 
-        it 'allows you to override approvals required' do
+        it 'allows you to set approvals required' do
           expect do
             post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), params: { approvals_required: 5 }
           end.to change { merge_request.reload.approvals_before_merge }.from(nil).to(5)
@@ -558,19 +537,6 @@ describe "API::MergeRequestApprovals with approval_rule enabled" do
           expect(response).to have_gitlab_http_status(201)
           expect(json_response['approvals_required']).to eq(5)
         end
-
-        context 'when project approvals are zero' do
-          before do
-            project.update!(approvals_before_merge: 0)
-          end
-
-          it 'does not include an error in the response' do
-            expect do
-              post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), params: { approvals_required: 0 }
-            end.to change {merge_request.reload.approvals_before_merge}.from(nil).to(0)
-            expect(json_response['message']).to eq(nil)
-          end
-        end
       end
 
       context 'when disable_overriding_approvers_per_merge_request is true on the project' do
@@ -578,7 +544,7 @@ describe "API::MergeRequestApprovals with approval_rule enabled" do
           project.update(disable_overriding_approvers_per_merge_request: true)
         end
 
-        it 'does not allow you to override approvals required' do
+        it 'does not allow you to set approvals_before_merge' do
           expect do
             post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), params: { approvals_required: 5 }
           end.not_to change { merge_request.reload.approvals_before_merge }
@@ -599,14 +565,14 @@ describe "API::MergeRequestApprovals with approval_rule enabled" do
     end
 
     context 'as a project admin' do
-      it_behaves_like 'user allowed to override approvals required' do
+      it_behaves_like 'user allowed to override approvals_before_merge' do
         let(:current_user) { user }
         let(:expected_approver_group_size) { 0 }
       end
     end
 
     context 'as a global admin' do
-      it_behaves_like 'user allowed to override approvals required' do
+      it_behaves_like 'user allowed to override approvals_before_merge' do
         let(:current_user) { admin }
         let(:expected_approver_group_size) { 1 }
       end

ee/spec/requests/api/merge_requests_spec.rb

@@ -149,55 +149,21 @@ describe API::MergeRequests do
             create_merge_request(approvals_before_merge: 1)
           end
 
-          it 'does not update approvals_before_merge' do
+          it 'does not set approvals_before_merge' do
             expect(json_response['approvals_before_merge']).to eq(nil)
           end
         end
 
-        context 'when the target project has approvals_before_merge set to zero' do
+        context 'when the target project has disable_overriding_approvers_per_merge_request set to false' do
           before do
             project.update(approvals_before_merge: 0)
             create_merge_request(approvals_before_merge: 1)
           end
 
-          it 'returns a 201' do
+          it 'sets approvals_before_merge' do
             expect(response).to have_gitlab_http_status(201)
-          end
-
-          it 'does not include an error in the response' do
             expect(json_response['message']).to eq(nil)
-          end
-        end
-
-        context 'when the target project has a non-zero approvals_before_merge' do
-          context 'when the approvals_before_merge param is less than or equal to the value in the target project' do
-            before do
-              project.update(approvals_before_merge: 2)
-              create_merge_request(approvals_before_merge: 1)
-            end
-
-            it 'returns a 400' do
-              expect(response).to have_gitlab_http_status(400)
-            end
-
-            it 'includes the error in the response' do
-              expect(json_response['message']['validate_approvals_before_merge']).not_to be_empty
-            end
-          end
-
-          context 'when the approvals_before_merge param is greater than the value in the target project' do
-            before do
-              project.update(approvals_before_merge: 1)
-              create_merge_request(approvals_before_merge: 2)
-            end
-
-            it 'returns a created status' do
-              expect(response).to have_gitlab_http_status(201)
-            end
-
-            it 'sets approvals_before_merge of the newly-created MR' do
-              expect(json_response['approvals_before_merge']).to eq(2)
-            end
+            expect(json_response['approvals_before_merge']).to eq(1)
           end
         end
       end