Merge branch '332587-constraint-status-check-branches' into 'master'

Constraint external status checks to protected branches

See merge request gitlab-org/gitlab!63422

ee/app/models/merge_requests/external_status_check.rb

@@ -8,6 +8,11 @@ module MergeRequests
     ignore_column :external_approval_rule_id, remove_with: '14.3', remove_after: '2021-09-22'
 
     scope :with_api_entity_associations, -> { preload(:protected_branches) }
+    scope :applicable_to_branch, ->(branch) do
+      includes(:protected_branches)
+        .references(:protected_branches)
+        .where('protected_branches.id IS NULL OR protected_branches.name = ?', branch)
+    end
 
     belongs_to :project
     has_and_belongs_to_many :protected_branches
@@ -16,6 +21,8 @@ module MergeRequests
     validates :name, uniqueness: { scope: :project_id }, presence: true
 
     def async_execute(data)
+      return unless protected_branches.none? || protected_branches.by_name(data[:object_attributes][:target_branch]).any?
+
       ApprovalRules::ExternalApprovalRulePayloadWorker.perform_async(self.id, payload_data(data))
     end
 

ee/lib/api/status_checks.rb

@@ -135,7 +135,7 @@ module API
 
           merge_request = find_merge_request_with_access(params[:merge_request_iid], :approve_merge_request)
 
-          present(paginate(merge_request.project.external_status_checks.all), with: Entities::MergeRequests::StatusCheck, merge_request: merge_request, sha: merge_request.source_branch_sha)
+          present(paginate(user_project.external_status_checks.applicable_to_branch(merge_request.target_branch)), with: Entities::MergeRequests::StatusCheck, merge_request: merge_request, sha: merge_request.source_branch_sha)
         end
       end
     end

ee/spec/models/merge_requests/external_status_check_spec.rb

@@ -22,6 +22,62 @@ RSpec.describe MergeRequests::ExternalStatusCheck, type: :model do
     end
   end
 
+  describe 'applicable_to_branch' do
+    let_it_be(:merge_request) { create(:merge_request) }
+    let_it_be(:check_belonging_to_different_project) { create(:external_status_check) }
+    let_it_be(:check_with_no_protected_branches) { create(:external_status_check, project: merge_request.project, protected_branches: []) }
+    let_it_be(:check_with_applicable_protected_branches) { create(:external_status_check, project: merge_request.project, protected_branches: [create(:protected_branch, name: merge_request.target_branch)]) }
+    let_it_be(:check_with_non_applicable_protected_branches) { create(:external_status_check, project: merge_request.project, protected_branches: [create(:protected_branch, name: 'testbranch')]) }
+
+    it 'returns the correct collection of checks' do
+      expect(merge_request.project.external_status_checks.applicable_to_branch(merge_request.target_branch)).to contain_exactly(check_with_no_protected_branches, check_with_applicable_protected_branches)
+    end
+  end
+
+  describe 'async_execute' do
+    let_it_be(:merge_request) { create(:merge_request) }
+
+    let(:data) do
+      {
+        object_attributes: {
+          target_branch: 'test'
+        }
+      }
+    end
+
+    subject { check.async_execute(data) }
+
+    context 'when list of protected branches is empty' do
+      let_it_be(:check) { create(:external_status_check, project: merge_request.project) }
+
+      it 'enqueues the status check' do
+        expect(ApprovalRules::ExternalApprovalRulePayloadWorker).to receive(:perform_async).once
+
+        subject
+      end
+    end
+
+    context 'when data target branch matches a protected branch' do
+      let_it_be(:check) { create(:external_status_check, project: merge_request.project, protected_branches: [create(:protected_branch, name: 'test')]) }
+
+      it 'enqueues the status check' do
+        expect(ApprovalRules::ExternalApprovalRulePayloadWorker).to receive(:perform_async).once
+
+        subject
+      end
+    end
+
+    context 'when data target branch does not match a protected branch' do
+      let_it_be(:check) { create(:external_status_check, project: merge_request.project, protected_branches: [create(:protected_branch, name: 'new-branch')]) }
+
+      it 'does not enqueue the status check' do
+        expect(ApprovalRules::ExternalApprovalRulePayloadWorker).to receive(:perform_async).never
+
+        subject
+      end
+    end
+  end
+
   describe 'approved?' do
     let_it_be(:rule) { create(:external_status_check) }
     let_it_be(:merge_request) { create(:merge_request) }

ee/spec/requests/api/status_checks_spec.rb

@@ -51,6 +51,8 @@ RSpec.describe API::StatusChecks do
       end
 
       context 'when merge request has received status check responses' do
+        let!(:non_applicable_check) { create(:external_status_check, project: project, protected_branches: [create(:protected_branch, name: 'different-branch')]) }
+        let!(:branch_specific_check) { create(:external_status_check, project: project, protected_branches: [create(:protected_branch, name: merge_request.target_branch)]) }
         let!(:status_check_response) { create(:status_check_response, external_status_check: rule, merge_request: merge_request, sha: sha) }
 
         it 'returns a 200' do
@@ -62,7 +64,7 @@ RSpec.describe API::StatusChecks do
         it 'returns the total number of status checks for the MRs project' do
           subject
 
-          expect(json_response.size).to eq(2)
+          expect(json_response.size).to eq(3)
         end
 
         it 'has the correct status values' do
@@ -70,6 +72,7 @@ RSpec.describe API::StatusChecks do
 
           expect(json_response[0]["status"]).to eq('approved')
           expect(json_response[1]["status"]).to eq('pending')
+          expect(json_response[2]["status"]).to eq('pending')
         end
       end
     end

ee/spec/services/ee/merge_requests/base_service_spec.rb

@@ -19,14 +19,12 @@ RSpec.describe MergeRequests::BaseService do
 
   subject { MergeRequests::CreateService.new(project: project, current_user: project.owner, params: params) }
 
-  context 'project has external status checks' do
-    let_it_be(:status_checks) { create_list(:external_status_check, 3, project: project) }
+  let_it_be(:status_checks) { create_list(:external_status_check, 3, project: project) }
 
-    it 'fires the correct number of compliance hooks' do
-      expect(project).to receive(:execute_external_compliance_hooks).once.and_call_original
+  it 'fires the correct number of compliance hooks' do
+    expect(project).to receive(:execute_external_compliance_hooks).once.and_call_original
 
-      subject.execute
-    end
+    subject.execute
   end
 
   describe '#filter_params' do