Merge branch '39060-disable-self-approval-at-the-instance-level-fix-problems' into 'master'

Disable self-approval at the Instance level - Fix approvals filtering

Closes #39060

See merge request gitlab-org/gitlab!25385

ee/app/models/ee/project.rb

@@ -719,6 +719,7 @@ module EE
       ::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? ||
         super
     end
+    alias_method :disable_overriding_approvers_per_merge_request?, :disable_overriding_approvers_per_merge_request
 
     def merge_requests_author_approval
       return super unless License.feature_available?(:admin_merge_request_approvers_rules)
@@ -727,6 +728,7 @@ module EE
 
       super
     end
+    alias_method :merge_requests_author_approval?, :merge_requests_author_approval
 
     def merge_requests_disable_committers_approval
       return super unless License.feature_available?(:admin_merge_request_approvers_rules)
@@ -734,6 +736,7 @@ module EE
       ::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? ||
         super
     end
+    alias_method :merge_requests_disable_committers_approval?, :merge_requests_disable_committers_approval
 
     def license_compliance
       strong_memoize(:license_compliance) { SCA::LicenseCompliance.new(self) }

ee/changelogs/unreleased/39060-disable-self-approval-at-the-instance-level-fix-problems.yml

+---
+title: Disable self-approval at the Instance level - Fix approvals filtering
+merge_request: 25385
+author:
+type: fixed

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -293,8 +293,9 @@ describe ApprovalMergeRequestRule do
           subject.group_users
         end
 
-        it 'does not cause queries' do
-          expect { subject.approvers }.not_to exceed_query_limit(0)
+        it 'does not perform any new queries when all users are loaded already' do
+          # single query is triggered for license check
+          expect { subject.approvers }.not_to exceed_query_limit(1)
         end
 
         it 'does not contain the author' do

ee/spec/models/approval_state_spec.rb

@@ -63,6 +63,28 @@ describe ApprovalState do
       it 'excludes authors' do
         expect(results).not_to include(merge_request.author)
       end
+
+      context 'when self approval is enabled on instance level' do
+        before do
+          stub_application_setting(prevent_merge_requests_author_approval: false)
+          stub_licensed_features(admin_merge_request_approvers_rules: true)
+        end
+
+        it 'excludes author' do
+          expect(results).not_to include(merge_request.author)
+        end
+      end
+
+      context 'when self approval is disabled on instance level' do
+        before do
+          stub_licensed_features(admin_merge_request_approvers_rules: true)
+          stub_application_setting(prevent_merge_requests_author_approval: true)
+        end
+
+        it 'excludes authors' do
+          expect(results).not_to include(merge_request.author)
+        end
+      end
     end
 
     context 'when self approval is enabled' do
@@ -71,24 +93,90 @@ describe ApprovalState do
       it 'includes author' do
         expect(results).to include(merge_request.author)
       end
+
+      context 'when self approval is enabled on instance level' do
+        before do
+          stub_application_setting(prevent_merge_requests_author_approval: false)
+          stub_licensed_features(admin_merge_request_approvers_rules: true)
+        end
+
+        it 'includes author' do
+          expect(results).to include(merge_request.author)
+        end
+      end
+
+      context 'when self approval is disabled on instance level' do
+        before do
+          stub_application_setting(prevent_merge_requests_author_approval: true)
+          stub_licensed_features(admin_merge_request_approvers_rules: true)
+        end
+
+        it 'excludes authors' do
+          expect(results).not_to include(merge_request.author)
+        end
+      end
     end
 
     context 'when committers approval is enabled' do
       let(:merge_requests_author_approval) { true }
       let(:merge_requests_disable_committers_approval) { false }
 
-      it 'excludes committers' do
+      it 'includes committers' do
         expect(results).to include(*committers)
       end
+
+      context 'when committers approval is enabled on instance level' do
+        before do
+          stub_application_setting(prevent_merge_requests_committers_approval: false)
+          stub_licensed_features(admin_merge_request_approvers_rules: true)
+        end
+
+        it 'includes committers' do
+          expect(results).to include(*committers)
+        end
+      end
+
+      context 'when committers approval is disabled on instance level' do
+        before do
+          stub_application_setting(prevent_merge_requests_committers_approval: true)
+          stub_licensed_features(admin_merge_request_approvers_rules: true)
+        end
+
+        it 'excludes committers' do
+          expect(results).not_to include(*committers)
+        end
+      end
     end
 
     context 'when committers approval is disabled' do
       let(:merge_requests_author_approval) { true }
       let(:merge_requests_disable_committers_approval) { true }
 
-      it 'includes committers' do
+      it 'excludes committers' do
         expect(results).not_to include(*committers)
       end
+
+      context 'when committers approval is enabled on instance level' do
+        before do
+          stub_application_setting(prevent_merge_requests_committers_approval: false)
+          stub_licensed_features(admin_merge_request_approvers_rules: true)
+        end
+
+        it 'excludes committers' do
+          expect(results).not_to include(*committers)
+        end
+      end
+
+      context 'when committers approval is disabled on instance level' do
+        before do
+          stub_application_setting(prevent_merge_requests_committers_approval: true)
+          stub_licensed_features(admin_merge_request_approvers_rules: true)
+        end
+
+        it 'excludes committers' do
+          expect(results).not_to include(*committers)
+        end
+      end
     end
   end
 

ee/spec/models/concerns/approval_rule_like_spec.rb

@@ -40,8 +40,9 @@ describe ApprovalRuleLike do
           subject.group_users
         end
 
-        it 'does not perform any queries when all users are loaded already' do
-          expect { subject.approvers }.not_to exceed_query_limit(0)
+        it 'does not perform any new queries when all users are loaded already' do
+          # single query is triggered for license check
+          expect { subject.approvers }.not_to exceed_query_limit(1)
         end
 
         it_behaves_like 'approvers contains the right users'

ee/spec/models/project_spec.rb

@@ -508,6 +508,7 @@ describe Project do
 
         it 'shows proper setting' do
           expect(project.send(setting)).to eq(final_setting)
+          expect(project.send("#{setting}?")).to eq(final_setting)
         end
       end
     end
@@ -556,6 +557,7 @@ describe Project do
 
         it 'shows proper setting' do
           expect(project.send(setting)).to eq(final_setting)
+          expect(project.send("#{setting}?")).to eq(final_setting)
         end
       end
     end

ee/spec/models/visible_approvable_spec.rb

@@ -78,13 +78,13 @@ describe VisibleApprovable do
     subject { resource.authors_can_approve? }
 
     it 'returns false when merge_requests_author_approval flag is off' do
-      is_expected.to be false
+      is_expected.to be_falsey
     end
 
     it 'returns true when merge_requests_author_approval flag is turned on' do
       project.update(merge_requests_author_approval: true)
 
-      is_expected.to be true
+      is_expected.to be_truthy
     end
   end
 end