Merge branch '290254-override-approvers-setting-does-not-reflect-actual-behavior' into 'master'

Ensure Project Approvals attributes (and API) casts to boolean

See merge request gitlab-org/gitlab!55492

changelogs/unreleased/290254-override-approvers-setting-does-not-reflect-actual-behavior.yml

+---
+title: Ensure Project Approvals API casts to boolean
+merge_request: 55492
+author:
+type: fixed

ee/app/models/ee/project.rb

@@ -534,7 +534,10 @@ module EE
     def require_password_to_approve
       super && password_authentication_enabled_for_web?
     end
-    alias_method :require_password_to_approve?, :require_password_to_approve
+
+    def require_password_to_approve?
+      !!require_password_to_approve
+    end
 
     def find_path_lock(path, exact_match: false, downstream: false)
       path_lock_finder = strong_memoize(:path_lock_finder) do
@@ -723,7 +726,10 @@ module EE
         ::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? || super
       end
     end
-    alias_method :disable_overriding_approvers_per_merge_request?, :disable_overriding_approvers_per_merge_request
+
+    def disable_overriding_approvers_per_merge_request?
+      !!disable_overriding_approvers_per_merge_request
+    end
 
     def merge_requests_author_approval
       strong_memoize(:merge_requests_author_approval) do
@@ -733,7 +739,10 @@ module EE
         super
       end
     end
-    alias_method :merge_requests_author_approval?, :merge_requests_author_approval
+
+    def merge_requests_author_approval?
+      !!merge_requests_author_approval
+    end
 
     def merge_requests_disable_committers_approval
       strong_memoize(:merge_requests_disable_committers_approval) do
@@ -742,7 +751,10 @@ module EE
         ::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? || super
       end
     end
-    alias_method :merge_requests_disable_committers_approval?, :merge_requests_disable_committers_approval
+
+    def merge_requests_disable_committers_approval?
+      !!merge_requests_disable_committers_approval
+    end
 
     def license_compliance(pipeline = latest_pipeline_with_reports(::Ci::JobArtifact.license_scanning_reports))
       SCA::LicenseCompliance.new(self, pipeline)

ee/lib/ee/api/entities/approval_settings.rb

@@ -7,11 +7,20 @@ module EE
         expose :approvers, using: EE::API::Entities::Approver
         expose :approver_groups, using: EE::API::Entities::ApproverGroup
         expose :approvals_before_merge
+
         expose :reset_approvals_on_push
-        expose :disable_overriding_approvers_per_merge_request
-        expose :merge_requests_author_approval
-        expose :merge_requests_disable_committers_approval
-        expose :require_password_to_approve
+
+        expose :disable_overriding_approvers_per_merge_request?,
+          as: :disable_overriding_approvers_per_merge_request
+
+        expose :merge_requests_author_approval?,
+          as: :merge_requests_author_approval
+
+        expose :merge_requests_disable_committers_approval?,
+          as: :merge_requests_disable_committers_approval
+
+        expose :require_password_to_approve?,
+          as: :require_password_to_approve
       end
     end
   end

ee/spec/models/project_spec.rb

@@ -677,6 +677,30 @@ RSpec.describe Project do
       end
     end
 
+    shared_examples 'a predicate wrapper method' do
+      where(:wrapped_method_return, :subject_return) do
+        true  | true
+        false | false
+        nil   | false
+      end
+
+      with_them do
+        it 'returns the expected boolean value' do
+          expect(project)
+            .to receive(wrapped_method)
+            .and_return(wrapped_method_return)
+
+          expect(project.send("#{wrapped_method}?")).to be(subject_return)
+        end
+      end
+    end
+
+    describe '#disable_overriding_approvers_per_merge_request?' do
+      it_behaves_like 'a predicate wrapper method' do
+        let(:wrapped_method) { :disable_overriding_approvers_per_merge_request }
+      end
+    end
+
     describe '#merge_requests_disable_committers_approval' do
       it_behaves_like 'setting modified by application setting' do
         let(:setting) { :merge_requests_disable_committers_approval }
@@ -684,6 +708,18 @@ RSpec.describe Project do
       end
     end
 
+    describe '#merge_requests_disable_committers_approval?' do
+      it_behaves_like 'a predicate wrapper method' do
+        let(:wrapped_method) { :merge_requests_disable_committers_approval }
+      end
+    end
+
+    describe '#require_password_to_approve?' do
+      it_behaves_like 'a predicate wrapper method' do
+        let(:wrapped_method) { :require_password_to_approve }
+      end
+    end
+
     describe '#merge_requests_author_approval' do
       let(:setting) { :merge_requests_author_approval }
       let(:application_setting) { :prevent_merge_requests_author_approval }
@@ -715,6 +751,12 @@ RSpec.describe Project do
         end
       end
     end
+
+    describe '#merge_requests_author_approval?' do
+      it_behaves_like 'a predicate wrapper method' do
+        let(:wrapped_method) { :merge_requests_author_approval }
+      end
+    end
   end
 
   describe '#has_active_hooks?' do

ee/spec/requests/api/project_approvals_spec.rb

@@ -20,6 +20,13 @@ RSpec.describe API::ProjectApprovals do
         get api(url, user)
       end
 
+      it 'returns expected boolean values for merge request related attributes' do
+        expect(json_response["disable_overriding_approvers_per_merge_request"]).to be false
+        expect(json_response["merge_requests_author_approval"]).to be false
+        expect(json_response["merge_requests_disable_committers_approval"]).to be false
+        expect(json_response["require_password_to_approve"]).to be false
+      end
+
       it 'returns 200 status' do
         expect(response).to have_gitlab_http_status(:ok)
       end