Merge branch '12055-public-mr-approval-rules-api' into 'master'

Implement public MR-level approval rules API

See merge request gitlab-org/gitlab-ee!15441

ee/app/policies/approval_merge_request_rule_policy.rb

@@ -4,7 +4,7 @@ class ApprovalMergeRequestRulePolicy < BasePolicy
   delegate { @subject.merge_request }
 
   condition(:editable) do
-    can?(:update_merge_request, @subject.merge_request)
+    can?(:update_merge_request, @subject.merge_request) && @subject.regular?
   end
 
   rule { editable }.enable :edit_approval_rule

ee/app/services/approval_rules/base_service.rb

@@ -3,21 +3,17 @@
 module ApprovalRules
   class BaseService < ::BaseService
     def execute
-      return error(['Prohibited']) unless can_edit?
+      return error(['Prohibited'], 403) unless can_edit?
 
-      filter_eligible_users!
-      filter_eligible_groups!
-
-      if rule.update(params)
-        rule.reset
-        success
-      else
-        error(rule.errors.messages)
-      end
+      action
     end
 
     private
 
+    def action
+      raise 'Not implemented'
+    end
+
     attr_reader :rule
 
     def can_edit?
@@ -27,17 +23,5 @@ module ApprovalRules
     def success(*args, &blk)
       super.tap { |hsh| hsh[:rule] = rule }
     end
-
-    def filter_eligible_users!
-      return unless params.key?(:user_ids)
-
-      params[:users] = project.members_among(User.id_in(params.delete(:user_ids)))
-    end
-
-    def filter_eligible_groups!
-      return unless params.key?(:group_ids)
-
-      params[:groups] = Group.id_in(params.delete(:group_ids)).public_or_visible_to_user(current_user)
-    end
   end
 end

ee/app/services/approval_rules/create_service.rb

@@ -2,6 +2,8 @@
 
 module ApprovalRules
   class CreateService < ::ApprovalRules::BaseService
+    include ::ApprovalRules::Updater
+
     # @param target [Project, MergeRequest]
     def initialize(target, user, params)
       @rule = target.approval_rules.build
@@ -12,7 +14,27 @@ module ApprovalRules
         params.reverse_merge!(rule_type: :report_approver)
       end
 
+      copy_approval_project_rule_properties(params) if target.is_a?(MergeRequest)
+
       super(@rule.project, user, params)
     end
+
+    private
+
+    def copy_approval_project_rule_properties(params)
+      return if params[:approval_project_rule_id].blank?
+
+      approval_project_rule = @rule.project.approval_rules.find_by_id(params[:approval_project_rule_id])
+
+      return if approval_project_rule.blank?
+
+      # Remove the following from params so when set they'll be ignored
+      params.delete(:user_ids)
+      params.delete(:group_ids)
+
+      params[:name] = approval_project_rule.name
+      params[:users] = approval_project_rule.users
+      params[:groups] = approval_project_rule.groups
+    end
   end
 end

ee/app/services/approval_rules/merge_request_rule_destroy_service.rb

+# frozen_string_literal: true
+
+module ApprovalRules
+  class MergeRequestRuleDestroyService < ::ApprovalRules::BaseService
+    def initialize(approval_rule, user)
+      @rule = approval_rule
+
+      super(@rule.project, user, params)
+    end
+
+    def action
+      @rule.destroy
+
+      if @rule.destroyed?
+        success
+      else
+        error(rule.errors.messages)
+      end
+    end
+  end
+end

ee/app/services/approval_rules/update_service.rb

@@ -2,6 +2,8 @@
 
 module ApprovalRules
   class UpdateService < ::ApprovalRules::BaseService
+    include ::ApprovalRules::Updater
+
     attr_reader :rule, :keep_existing_hidden_groups
 
     def initialize(approval_rule, user, params)

ee/app/services/concerns/approval_rules/updater.rb

+# frozen_string_literal: true
+
+module ApprovalRules
+  module Updater
+    def action
+      filter_eligible_users!
+      filter_eligible_groups!
+
+      if rule.update(params)
+        rule.reset
+        success
+      else
+        error(rule.errors.messages)
+      end
+    end
+
+    private
+
+    def filter_eligible_users!
+      return unless params.key?(:user_ids)
+
+      params[:users] = project.members_among(User.id_in(params.delete(:user_ids)))
+    end
+
+    def filter_eligible_groups!
+      return unless params.key?(:group_ids)
+
+      params[:groups] = Group.id_in(params.delete(:group_ids)).public_or_visible_to_user(current_user)
+    end
+  end
+end

ee/changelogs/unreleased/12055-public-mr-approval-rules-api.yml

+---
+title: Implement public MR-level approval rules API
+merge_request: 15441
+author:
+type: added

ee/lib/api/helpers/approval_helpers.rb

-# frozen_string_literal: true
-
-module API
-  module Helpers
-    module ApprovalHelpers
-      def present_approval(merge_request)
-        present merge_request.approval_state, with: ::EE::API::Entities::ApprovalState, current_user: current_user
-      end
-    end
-  end
-end

ee/lib/api/helpers/project_approval_rules_helpers.rb

@@ -40,7 +40,7 @@ module API
         if result[:status] == :success
           present result[:rule], with: present_with, current_user: current_user
         else
-          render_api_error!(result[:message], 400)
+          render_api_error!(result[:message], result[:http_status] || 400)
         end
       end
 
@@ -54,7 +54,7 @@ module API
         if result[:status] == :success
           present result[:rule], with: present_with, current_user: current_user
         else
-          render_api_error!(result[:message], 400)
+          render_api_error!(result[:message], result[:http_status] || 400)
         end
       end
 

ee/lib/api/merge_request_approval_rules.rb

+# frozen_string_literal: true
+
+module API
+  class MergeRequestApprovalRules < ::Grape::API
+    before { authenticate_non_get! }
+
+    ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) }
+
+    helpers do
+      def find_merge_request_approval_rule(merge_request, id)
+        merge_request.approval_rules.find_by_id!(id)
+      end
+    end
+
+    params do
+      requires :id, type: String, desc: 'The ID of a project'
+      requires :merge_request_iid, type: Integer, desc: 'The IID of a merge request'
+    end
+    resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
+      segment ':id/merge_requests/:merge_request_iid/approval_rules' do
+        desc 'Get all merge request approval rules' do
+          success EE::API::Entities::MergeRequestApprovalRule
+        end
+        get do
+          merge_request = find_merge_request_with_access(params[:merge_request_iid])
+
+          present merge_request.approval_rules, with: EE::API::Entities::MergeRequestApprovalRule, current_user: current_user
+        end
+
+        desc 'Create new merge request approval rules' do
+          success EE::API::Entities::MergeRequestApprovalRule
+        end
+        params do
+          requires :name, type: String, desc: 'The name of the approval rule'
+          requires :approvals_required, type: Integer, desc: 'The number of required approvals for this rule'
+          optional :approval_project_rule_id, type: Integer, desc: 'The ID of a project-level approval rule'
+          optional :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule'
+          optional :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule'
+        end
+        post do
+          merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers)
+          result = ::ApprovalRules::CreateService.new(merge_request, current_user, declared_params(include_missing: false)).execute
+
+          if result[:status] == :success
+            present result[:rule], with: EE::API::Entities::MergeRequestApprovalRule, current_user: current_user
+          else
+            render_api_error!(result[:message], result[:http_status] || 400)
+          end
+        end
+
+        segment ':approval_rule_id' do
+          desc 'Update merge request approval rule' do
+            success EE::API::Entities::MergeRequestApprovalRule
+          end
+          params do
+            requires :approval_rule_id, type: Integer, desc: 'The ID of an approval rule'
+            optional :name, type: String, desc: 'The name of the approval rule'
+            optional :approvals_required, type: Integer, desc: 'The number of required approvals for this rule'
+            optional :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule'
+            optional :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule'
+            optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed'
+          end
+          put do
+            merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers)
+            params = declared_params(include_missing: false)
+            approval_rule = find_merge_request_approval_rule(merge_request, params.delete(:approval_rule_id))
+            result = ::ApprovalRules::UpdateService.new(approval_rule, current_user, params).execute
+
+            if result[:status] == :success
+              present result[:rule], with: EE::API::Entities::MergeRequestApprovalRule, current_user: current_user
+            else
+              render_api_error!(result[:message], result[:http_status] || 400)
+            end
+          end
+
+          desc 'Destroy merge request approval rule'
+          params do
+            requires :approval_rule_id, type: Integer, desc: 'The ID of an approval_rule'
+          end
+          delete do
+            merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers)
+            approval_rule = find_merge_request_approval_rule(merge_request, params[:approval_rule_id])
+
+            destroy_conditionally!(approval_rule) do |rule|
+              result = ::ApprovalRules::MergeRequestRuleDestroyService.new(rule, current_user).execute
+
+              if result[:status] == :error
+                render_api_error!(result[:message], result[:http_status] || 400)
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end

ee/lib/api/merge_request_approvals.rb

@@ -6,8 +6,11 @@ module API
 
     ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) }
 
-    helpers ::API::Helpers::ApprovalHelpers
     helpers do
+      def present_approval(merge_request)
+        present merge_request.approval_state, with: ::EE::API::Entities::ApprovalState, current_user: current_user
+      end
+
       def handle_merge_request_errors!(errors)
         if errors.has_key? :project_access
           error!(errors[:project_access], 422)
@@ -50,13 +53,13 @@ module API
         end
 
         desc 'List approval rules for merge request', {
-          success: ::EE::API::Entities::MergeRequestApprovalRules,
+          success: ::EE::API::Entities::MergeRequestApprovalSettings,
           hidden: true
         }
         get 'approval_settings' do
           merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
-          present merge_request.approval_state, with: ::EE::API::Entities::MergeRequestApprovalRules, current_user: current_user
+          present merge_request.approval_state, with: ::EE::API::Entities::MergeRequestApprovalSettings, current_user: current_user
         end
 
         desc 'Change approval-related configuration' do

ee/lib/ee/api/api.rb

@@ -36,6 +36,7 @@ module EE
         mount ::API::ProjectApprovals
         mount ::API::Vulnerabilities
         mount ::API::MergeRequestApprovals
+        mount ::API::MergeRequestApprovalRules
         mount ::API::ProjectAliases
         mount ::API::Dependencies
 

ee/lib/ee/api/entities.rb

@@ -317,41 +317,55 @@ module EE
         expose :id, :name, :rule_type
       end
 
-      class ApprovalSettingRule < ApprovalRuleShort
+      class ApprovalRule < ApprovalRuleShort
         def initialize(object, options = {})
           presenter = ::ApprovalRulePresenter.new(object, current_user: options[:current_user])
           super(presenter, options)
         end
 
-        expose :approvers, using: ::API::Entities::UserBasic
+        expose :approvers, as: :eligible_approvers, using: ::API::Entities::UserBasic
         expose :approvals_required
         expose :users, using: ::API::Entities::UserBasic
         expose :groups, using: ::API::Entities::Group
         expose :contains_hidden_groups?, as: :contains_hidden_groups
       end
 
-      class ApprovalRule < ApprovalSettingRule
-        expose :approvers, as: :eligible_approvers, using: ::API::Entities::UserBasic, override: true
+      # Being used in private project-level approvals API.
+      # This overrides the `eligible_approvers` to be exposed as `approvers`.
+      #
+      # To be removed in https://gitlab.com/gitlab-org/gitlab-ee/issues/13574.
+      class ApprovalSettingRule < ApprovalRule
+        expose :approvers, using: ::API::Entities::UserBasic, override: true
       end
 
-      class MergeRequestApprovalRule < ApprovalSettingRule
+      class MergeRequestApprovalRule < ApprovalRule
         class SourceRule < Grape::Entity
           expose :approvals_required
         end
 
-        expose :approved_approvers, as: :approved_by, using: ::API::Entities::UserBasic
-        expose :code_owner
         expose :source_rule, using: SourceRule
+      end
+
+      # Being used in private MR-level approvals API.
+      # This overrides the `eligible_approvers` to be exposed as `approvers` and
+      # include additional properties.
+      #
+      # To be made public in https://gitlab.com/gitlab-org/gitlab-ee/issues/13712
+      # and the `approvers` override can be removed.
+      class MergeRequestApprovalSettingRule < MergeRequestApprovalRule
+        expose :approvers, using: ::API::Entities::UserBasic, override: true
+        expose :code_owner
+        expose :approved_approvers, as: :approved_by, using: ::API::Entities::UserBasic
         expose :approved?, as: :approved
       end
 
       # Decorates ApprovalState
-      class MergeRequestApprovalRules < Grape::Entity
+      class MergeRequestApprovalSettings < Grape::Entity
         expose :approval_rules_overwritten do |approval_state|
           approval_state.approval_rules_overwritten?
         end
 
-        expose :wrapped_approval_rules, as: :rules, using: MergeRequestApprovalRule
+        expose :wrapped_approval_rules, as: :rules, using: MergeRequestApprovalSettingRule
       end
 
       # Decorates Project

ee/spec/fixtures/api/schemas/public_api/v4/merge_request_approval_rule.json

+{
+  "type": "object",
+  "properties": {
+    "id": { "type": "integer" },
+    "name": { "type": "string" },
+    "approvals_required": { "type": "integer" },
+    "contains_hidden_groups": { "type": "boolean" },
+    "rule_type": { "type": "string" },
+    "source_rule": {
+      "type":["null", "object"],
+      "properties": {}
+    },
+    "eligible_approvers": {
+      "type": "array",
+      "items": {
+        "type": "object",
+        "properties": {}
+      }
+    },
+    "groups": {
+      "type": "array",
+      "items": {
+        "type": "object",
+        "properties": {}
+      }
+    },
+    "users": {
+      "type": "array",
+      "items": {
+        "type": "object",
+        "properties": {}
+      }
+    }
+  },
+  "additionalProperties": false
+}

ee/spec/fixtures/api/schemas/public_api/v4/merge_request_approval_rules.json

+{
+  "type": "array",
+  "items": { "$ref": "./merge_request_approval_rule.json" }
+}

ee/spec/policies/approval_merge_request_rule_policy_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 describe ApprovalMergeRequestRulePolicy do
   let(:merge_request) { create(:merge_request) }
-  let!(:approval_rule) { create(:approval_merge_request_rule, merge_request: merge_request) }
+  let(:approval_rule) { create(:approval_merge_request_rule, merge_request: merge_request) }
 
   def permissions(user, approval_rule)
     described_class.new(user, approval_rule)
@@ -14,10 +14,18 @@ describe ApprovalMergeRequestRulePolicy do
     it 'allows updating approval rule' do
       expect(permissions(merge_request.author, approval_rule)).to be_allowed(:edit_approval_rule)
     end
+
+    context 'when rule is not regular type' do
+      let(:approval_rule) { create(:code_owner_rule, merge_request: merge_request) }
+
+      it 'disallows updating approval rule' do
+        expect(permissions(merge_request.author, approval_rule)).to be_disallowed(:edit_approval_rule)
+      end
+    end
   end
 
   context 'when user cannot update merge request' do
-    it 'disallow updating approval rule' do
+    it 'disallows updating approval rule' do
       expect(permissions(create(:user), approval_rule)).to be_disallowed(:edit_approval_rule)
     end
   end

ee/spec/services/approval_rules/create_service_spec.rb

@@ -106,38 +106,59 @@ describe ApprovalRules::CreateService do
     it_behaves_like "creatable"
 
     context 'when project rule id is present' do
-      let(:project_rule) { create(:approval_project_rule, project: project) }
+      let(:project_rule) do
+        create(
+          :approval_project_rule,
+          project: project,
+          name: 'bar',
+          approvals_required: 1,
+          users: [create(:user)],
+          groups: [create(:group)]
+        )
+      end
 
-      it 'associates with project rule' do
-        result = described_class.new(target, user, {
+      let(:result) do
+        described_class.new(target, user, {
           name: 'foo',
-          approvals_required: 1,
-          approval_project_rule_id: project_rule.id
+          approvals_required: 0,
+          approval_project_rule_id: project_rule.id,
+          user_ids: [],
+          group_ids: []
         }).execute
+      end
 
-        expect(result[:status]).to eq(:success)
-
-        rule = result[:rule]
+      let(:rule) { result[:rule] }
 
+      it 'associates with project rule' do
+        expect(result[:status]).to eq(:success)
+        expect(rule.approvals_required).to eq(0)
         expect(rule.approval_project_rule).to eq(project_rule)
       end
-    end
 
-    context "when project rule id is not the same as MR's project" do
-      let(:project_rule) { create(:approval_project_rule) }
+      it 'copies properties from the project rule' do
+        expect(rule.name).to eq(project_rule.name)
+        expect(rule.users).to match(project_rule.users)
+        expect(rule.groups).to match(project_rule.groups)
+      end
 
-      it 'ignores assignment' do
-        result = described_class.new(target, user, {
-          name: 'foo',
-          approvals_required: 1,
-          approval_project_rule_id: project_rule.id
-        }).execute
+      context 'when project rule is under the same project as MR' do
+        let(:another_project) { create(:project) }
 
-        expect(result[:status]).to eq(:success)
+        before do
+          project_rule.update!(project: another_project)
+        end
 
-        rule = result[:rule]
+        it 'ignores assignment' do
+          expect(result[:status]).to eq(:success)
+          expect(rule.approvals_required).to eq(0)
+          expect(rule.approval_project_rule).to eq(nil)
+        end
 
-        expect(rule.approval_project_rule).to eq(nil)
+        it 'does not copy properties from project rule' do
+          expect(rule.name).to eq('foo')
+          expect(rule.users).to be_empty
+          expect(rule.groups).to be_empty
+        end
       end
     end
   end

ee/spec/services/approval_rules/merge_request_rule_destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ApprovalRules::MergeRequestRuleDestroyService do
+  let(:rule) { create(:approval_merge_request_rule) }
+  let(:user) { create(:user) }
+
+  subject(:result) { described_class.new(rule, user).execute }
+
+  before do
+    allow(Ability).to receive(:allowed?).and_call_original
+    allow(Ability)
+      .to receive(:allowed?)
+      .with(user, :edit_approval_rule, rule)
+      .at_least(:once)
+      .and_return(can_edit?)
+  end
+
+  context 'user cannot edit approval rule' do
+    let(:can_edit?) { false }
+
+    it 'returns error status' do
+      expect(result[:status]).to eq(:error)
+    end
+  end
+
+  context 'user can edit approval rule' do
+    let(:can_edit?) { true }
+
+    context 'when rule successfully deleted' do
+      it 'returns successful status' do
+        expect(result[:status]).to eq(:success)
+      end
+    end
+
+    context 'when rule not successfully deleted' do
+      before do
+        allow(rule).to receive(:destroyed?).and_return(false)
+      end
+
+      it 'returns error status' do
+        expect(result[:status]).to eq(:error)
+      end
+    end
+  end
+end