Use max of approvals_before_merge between project and MR at runtime

Previously, MR#approvals_before_merge is nil when it is <= project
(from `clamp_approvals_before_merge`).
This changed in f480d145,
which would always persist a min value.
This caused regression that updating MR would fail if project's
value is increased later on.

The relationship between value being nil and validation can be easily
overlooked.

Removing validation and taking the max of the two makes logic clearer.

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)
-      end
+        merge_request.target_project.approvals_before_merge = project_db_value
       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