Merge branch '328211-list-mr-external-approvals' into 'master'

List all external status checks and their statuses

See merge request gitlab-org/gitlab!61300

db/migrate/20210510083845_add_sha_to_status_check_response.rb

+# frozen_string_literal: true
+
+class AddShaToStatusCheckResponse < ActiveRecord::Migration[6.0]
+  def up
+    execute('DELETE FROM status_check_responses')
+
+    add_column :status_check_responses, :sha, :binary, null: false # rubocop:disable Rails/NotNullColumn
+  end
+
+  def down
+    remove_column :status_check_responses, :sha
+  end
+end

db/schema_migrations/20210510083845

+307e45d581c48b6f571fc8fa2a00dfd4360296560ee2b320540314b8f9f9e02c
\ No newline at end of file

db/structure.sql

@@ -18108,7 +18108,8 @@ ALTER SEQUENCE sprints_id_seq OWNED BY sprints.id;
 CREATE TABLE status_check_responses (
     id bigint NOT NULL,
     merge_request_id bigint NOT NULL,
-    external_approval_rule_id bigint NOT NULL
+    external_approval_rule_id bigint NOT NULL,
+    sha bytea NOT NULL
 );
 
 CREATE SEQUENCE status_check_responses_id_seq

doc/api/status_checks.md

+---
+stage: Manage
+group: Compliance
+info: "To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments"
+type: reference, api
+---
+
+# External Status Checks API **(ULTIMATE)**
+
+> - [Introduced](https://gitlab.com/groups/gitlab-org/-/epics/3869) in GitLab 14.0.
+> - It's [deployed behind a feature flag](../user/feature_flags.md), disabled by default.
+> - It's disabled on GitLab.com.
+> - It's not recommended for production use.
+> - To use it in GitLab self-managed instances, ask a GitLab administrator to [enable it](#enable-or-disable-status-checks). **(ULTIMATE SELF)**
+ 
+WARNING:
+This feature might not be available to you. Check the **version history** note above for details.
+
+## List status checks for a merge request
+
+For a single merge request, list the external status checks that apply to it and their status.
+
+```plaintext
+GET /projects/:id/merge_requests/:merge_request_iid/status_checks
+```
+
+**Parameters:**
+
+| Attribute                | Type    | Required | Description                |
+| ------------------------ | ------- | -------- | -------------------------- |
+| `id`                     | integer | yes      | ID of a project            |
+| `merge_request_iid`      | integer | yes      | IID of a merge request     |
+
+```json
+[
+    {
+        "id": 2,
+        "name": "Rule 1",
+        "external_url": "https://gitlab.com/test-endpoint",
+        "status": "approved"
+    },
+    {
+        "id": 1,
+        "name": "Rule 2",
+        "external_url": "https://gitlab.com/test-endpoint-2",
+        "status": "pending"
+    }
+]
+```
+
+## Set approval status of an external status check
+
+For a single merge request, use the API to inform GitLab that a merge request has been approved by an external service.
+
+```plaintext
+POST /projects/:id/merge_requests/:merge_request_iid/status_check_responses
+```
+
+**Parameters:**
+
+| Attribute                | Type    | Required | Description                            |
+| ------------------------ | ------- | -------- | -------------------------------------- |
+| `id`                     | integer | yes      | ID of a project                    |
+| `merge_request_iid`      | integer | yes      | IID of a merge request             |
+| `sha`                    | string  | yes      | SHA at `HEAD` of the source branch |
+
+NOTE:
+`sha` must be the SHA at the `HEAD` of the merge request's source branch.
+
+## Enable or disable status checks **(ULTIMATE SELF)**
+
+Status checks are under development and not ready for production use. It is
+deployed behind a feature flag that is **disabled by default**.
+[GitLab administrators with access to the GitLab Rails console](../administration/feature_flags.md)
+can enable it.
+
+To enable it:
+
+```ruby
+# For the instance
+Feature.enable(:ff_compliance_approval_gates)
+# For a single project
+Feature.enable(:ff_compliance_approval_gates, Project.find(<project id>))
+```
+
+To disable it:
+
+```ruby
+# For the instance
+Feature.disable(:ff_compliance_approval_gates)
+# For a single project
+Feature.disable(:ff_compliance_approval_gates, Project.find(<project id>)
+```

ee/app/models/approval_rules/external_approval_rule.rb

@@ -15,6 +15,10 @@ module ApprovalRules
       ApprovalRules::ExternalApprovalRulePayloadWorker.perform_async(self.id, payload_data(data))
     end
 
+    def approved?(merge_request, sha)
+      merge_request.status_check_responses.where(external_approval_rule: self, sha: sha).exists?
+    end
+
     def to_h
       {
         id: self.id,

ee/app/models/merge_requests/status_check_response.rb

@@ -4,10 +4,15 @@ module MergeRequests
   class StatusCheckResponse < ApplicationRecord
     self.table_name = 'status_check_responses'
 
+    include ShaAttribute
+
+    sha_attribute :sha
+
     belongs_to :merge_request
     belongs_to :external_approval_rule, class_name: 'ApprovalRules::ExternalApprovalRule'
 
     validates :merge_request, presence: true
     validates :external_approval_rule, presence: true
+    validates :sha, presence: true
   end
 end

ee/lib/api/entities/merge_requests/status_check.rb

+# frozen_string_literal: true
+
+module API
+  module Entities
+    module MergeRequests
+      class StatusCheck < Grape::Entity
+        expose :id
+        expose :name
+        expose :external_url
+        expose :status
+
+        def status
+          object.approved?(options[:merge_request], options[:sha]) ? 'approved' : 'pending'
+        end
+      end
+    end
+  end
+end

ee/lib/api/status_checks.rb

@@ -2,12 +2,14 @@
 
 module API
   class StatusChecks < ::API::Base
+    include PaginationParams
+
     feature_category :compliance_management
 
     before { authenticate! }
 
     resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
-      segment ':id/merge_requests/:merge_request_iid/status_check_responses' do
+      segment ':id/merge_requests/:merge_request_iid' do
         desc 'Externally approve a merge request' do
           detail 'This feature was introduced in 13.12 and is gated behind the :ff_compliance_approval_gates feature flag.'
           success Entities::MergeRequests::StatusCheckResponse
@@ -18,17 +20,28 @@ module API
           requires :external_approval_rule_id, type: Integer, desc: 'The ID of a merge request rule'
           requires :sha, type: String, desc: 'The current SHA at HEAD of the merge request.'
         end
-        post do
+        post 'status_check_responses' do
           not_found! unless ::Feature.enabled?(:ff_compliance_approval_gates, user_project)
 
           merge_request = find_merge_request_with_access(params[:merge_request_iid], :approve_merge_request)
 
           check_sha_param!(params, merge_request)
 
-          approval = merge_request.status_check_responses.create(external_approval_rule_id: params[:external_approval_rule_id])
+          approval = merge_request.status_check_responses.create!(external_approval_rule_id: params[:external_approval_rule_id], sha: params[:sha])
 
           present(approval, with: Entities::MergeRequests::StatusCheckResponse)
         end
+
+        desc 'List all status checks for a merge request and their state.' do
+          detail 'This feature was introduced in 13.12 and is gated behind the :ff_compliance_approval_gates feature flag.'
+        end
+        get 'status_checks' do
+          not_found! unless ::Feature.enabled?(:ff_compliance_approval_gates, user_project)
+
+          merge_request = find_merge_request_with_access(params[:merge_request_iid], :approve_merge_request)
+
+          present(paginate(merge_request.project.external_approval_rules.all), with: Entities::MergeRequests::StatusCheck, merge_request: merge_request, sha: merge_request.source_branch_sha)
+        end
       end
     end
   end

ee/spec/models/approval_rules/external_approval_rule_spec.rb

@@ -21,4 +21,39 @@ RSpec.describe ApprovalRules::ExternalApprovalRule, type: :model do
       expect(subject.to_h).to eq({ id: subject.id, name: subject.name, external_url: subject.external_url })
     end
   end
+
+  describe 'approved?' do
+    let_it_be(:rule) { create(:external_approval_rule) }
+    let_it_be(:merge_request) { create(:merge_request) }
+
+    let(:project) { merge_request.source_project }
+
+    subject { rule.approved?(merge_request, merge_request.source_branch_sha) }
+
+    context 'when a rule has a positive status check response' do
+      let_it_be(:status_check_response) { create(:status_check_response, merge_request: merge_request, external_approval_rule: rule, sha: merge_request.source_branch_sha) }
+
+      it { is_expected.to be true }
+
+      context 'when a rule also has a positive check response from an old sha' do
+        before do
+          create(:status_check_response, merge_request: merge_request, external_approval_rule: rule, sha: 'abc1234')
+        end
+
+        it { is_expected.to be true }
+      end
+    end
+
+    context 'when a rule has no positive status check response' do
+      it { is_expected.to be false }
+    end
+
+    context 'when a rule has a positive status check response from an old sha' do
+      before do
+        create(:status_check_response, merge_request: merge_request, external_approval_rule: rule, sha: 'abc123')
+      end
+
+      it { is_expected.to be false }
+    end
+  end
 end

ee/spec/models/merge_requests/status_check_response_spec.rb

@@ -10,4 +10,5 @@ RSpec.describe MergeRequests::StatusCheckResponse, type: :model do
 
   it { is_expected.to validate_presence_of(:merge_request) }
   it { is_expected.to validate_presence_of(:external_approval_rule) }
+  it { is_expected.to validate_presence_of(:sha) }
 end

ee/spec/requests/api/status_checks_spec.rb

@@ -5,23 +5,72 @@ require 'spec_helper'
 RSpec.describe API::StatusChecks do
   include AccessMatchersForRequest
 
-  describe 'POST :id/:merge_requests/:merge_request_iid/status_check_responses' do
-    let_it_be(:project) { create(:project, :repository) }
-    let_it_be(:rule) { create(:external_approval_rule, project: project) }
-    let_it_be(:merge_request) { create(:merge_request, source_project: project) }
-    let_it_be(:project_maintainer) { create(:user) }
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:rule) { create(:external_approval_rule, project: project) }
+  let_it_be(:rule_2) { create(:external_approval_rule, project: project) }
 
-    let(:sha) { merge_request.source_branch_sha }
-    let(:user) { project_maintainer }
+  let_it_be(:merge_request) { create(:merge_request, source_project: project) }
+  let_it_be(:project_maintainer) { create(:user) }
 
-    subject { post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/status_check_responses", user), params: { external_approval_rule_id: rule.id, sha: sha } }
+  let(:sha) { merge_request.source_branch_sha }
+  let(:user) { project_maintainer }
+
+  subject { post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/status_check_responses", user), params: { external_approval_rule_id: rule.id, sha: sha } }
+
+  describe 'permissions' do
+    it { expect { subject }.to be_allowed_for(:maintainer).of(project) }
+    it { expect { subject }.to be_allowed_for(:developer).of(project) }
+    it { expect { subject }.to be_denied_for(:reporter).of(project) }
+  end
+
+  describe 'GET :id/merge_requests/:merge_request_iid/status_checks' do
+    subject { get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/status_checks", user), params: { external_approval_rule_id: rule.id, sha: sha } }
+
+    context 'feature flag is disabled' do
+      before do
+        stub_feature_flags(ff_compliance_approval_gates: false)
+      end
 
-    describe 'permissions' do
-      it { expect { subject }.to be_allowed_for(:maintainer).of(project) }
-      it { expect { subject }.to be_allowed_for(:developer).of(project) }
-      it { expect { subject }.to be_denied_for(:reporter).of(project) }
+      it 'returns a not found error' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
     end
 
+    context 'when current_user has access' do
+      before do
+        project.add_user(project_maintainer, :maintainer)
+      end
+
+      context 'when merge request has received status check responses' do
+        let!(:status_check_response) { create(:status_check_response, external_approval_rule: rule, merge_request: merge_request, sha: sha) }
+
+        it 'returns a 200' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:success)
+        end
+
+        it 'returns the total number of status checks for the MRs project' do
+          subject
+
+          expect(json_response.size).to eq(2)
+        end
+
+        it 'has the correct status values' do
+          subject
+
+          expect(json_response[0]["status"]).to eq('approved')
+          expect(json_response[1]["status"]).to eq('pending')
+        end
+      end
+    end
+  end
+
+  describe 'POST :id/:merge_requests/:merge_request_iid/status_check_responses' do
+    subject { post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/status_check_responses", user), params: { external_approval_rule_id: rule.id, sha: sha } }
+
     context 'feature flag is disabled' do
       before do
         stub_feature_flags(ff_compliance_approval_gates: false)