Merge branch '323127-add-more-mr-approval-settings-to-group-level' into 'master'

Add more settings to group MR approval settings

See merge request gitlab-org/gitlab!56215

changelogs/unreleased/323127-add-more-mr-approval-settings-to-group-level.yml

+---
+title: Add more settings to group MR approval settings
+merge_request: 56215
+author:
+type: added

db/migrate/20210310111009_add_settings_to_group_merge_request_approval_settings.rb

+# frozen_string_literal: true
+
+class AddSettingsToGroupMergeRequestApprovalSettings < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    change_table(:group_merge_request_approval_settings, bulk: true) do |t|
+      t.column :allow_committer_approval, :boolean, null: false, default: false
+      t.column :allow_overrides_to_approver_list_per_merge_request, :boolean, null: false, default: false
+      t.column :retain_approvals_on_push, :boolean, null: false, default: false
+      t.column :require_password_to_approve, :boolean, null: false, default: false
+    end
+  end
+end

db/schema_migrations/20210310111009

+8b5a69947c44c9c1050f4989e3b373d3eb87832111d0202992c7dd992032c9d1
\ No newline at end of file

db/structure.sql

@@ -13270,7 +13270,11 @@ CREATE TABLE group_merge_request_approval_settings (
     created_at timestamp with time zone NOT NULL,
     updated_at timestamp with time zone NOT NULL,
     group_id bigint NOT NULL,
-    allow_author_approval boolean DEFAULT false NOT NULL
+    allow_author_approval boolean DEFAULT false NOT NULL,
+    allow_committer_approval boolean DEFAULT false NOT NULL,
+    allow_overrides_to_approver_list_per_merge_request boolean DEFAULT false NOT NULL,
+    retain_approvals_on_push boolean DEFAULT false NOT NULL,
+    require_password_to_approve boolean DEFAULT false NOT NULL
 );
 
 CREATE TABLE group_repository_storage_moves (

ee/app/models/group_merge_request_approval_setting.rb

@@ -6,9 +6,12 @@ class GroupMergeRequestApprovalSetting < ApplicationRecord
   belongs_to :group, inverse_of: :group_merge_request_approval_setting
 
   validates :group, presence: true
-  validates :allow_author_approval, inclusion: { in: [true, false], message: _('must be a boolean value') }
+  validates :allow_author_approval,
+            :allow_committer_approval,
+            :allow_overrides_to_approver_list_per_merge_request,
+            :retain_approvals_on_push,
+            :require_password_to_approve,
+            inclusion: { in: [true, false], message: _('must be a boolean value') }
 
-  scope :find_or_initialize_by_group, ->(group) {
-    find_or_initialize_by(group: group)
-  }
+  scope :find_or_initialize_by_group, ->(group) { find_or_initialize_by(group: group) }
 end

ee/lib/api/entities/group_merge_request_approval_setting.rb

@@ -4,6 +4,10 @@ module API
   module Entities
     class GroupMergeRequestApprovalSetting < Grape::Entity
       expose :allow_author_approval
+      expose :allow_committer_approval
+      expose :allow_overrides_to_approver_list_per_merge_request
+      expose :retain_approvals_on_push
+      expose :require_password_to_approve
     end
   end
 end

ee/lib/api/group_merge_request_approval_settings.rb

@@ -7,6 +7,8 @@ module API
     before do
       authenticate!
       not_found! unless ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, user_group)
+
+      authorize! :admin_merge_request_approval_settings, user_group
     end
 
     params do
@@ -19,8 +21,6 @@ module API
           success ::API::Entities::GroupMergeRequestApprovalSetting
         end
         get do
-          authorize! :admin_merge_request_approval_settings, user_group
-
           setting = GroupMergeRequestApprovalSetting.find_or_initialize_by_group(user_group)
 
           present setting, with: ::API::Entities::GroupMergeRequestApprovalSetting
@@ -32,10 +32,20 @@ module API
         end
         params do
           optional :allow_author_approval, type: Boolean, desc: 'Allow authors to self-approve merge requests'
+          optional :allow_committer_approval, type: Boolean, desc: 'Allow committers to approve merge requests'
+          optional :allow_overrides_to_approver_list_per_merge_request,
+                   type: Boolean, desc: 'Allow overrides to approver list per merge request'
+          optional :retain_approvals_on_push, type: Boolean, desc: 'Retain approval count on a new push'
+          optional :require_password_to_approve,
+                   type: Boolean, desc: 'Require approver to authenticate before approving'
+
+          at_least_one_of :allow_author_approval,
+                          :allow_committer_approval,
+                          :allow_overrides_to_approver_list_per_merge_request,
+                          :retain_approvals_on_push,
+                          :require_password_to_approve
         end
         put do
-          authorize! :admin_merge_request_approval_settings, user_group
-
           setting_params = declared_params(include_missing: false)
 
           response = ::MergeRequestApprovalSettings::UpdateService

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

 {
   "type": "object",
   "properties": {
-    "allow_author_approval": { "type": "boolean" }
+    "allow_author_approval": { "type": "boolean" },
+    "allow_committer_approval": { "type": "boolean" },
+    "allow_overrides_to_approver_list_per_merge_request": { "type": "boolean" },
+    "retain_approvals_on_push": { "type": "boolean" },
+    "require_password_to_approve": { "type": "boolean" }
   },
   "additionalProperties": false
 }

ee/spec/lib/api/entities/group_merge_request_approval_setting_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe API::Entities::GroupMergeRequestApprovalSetting do
+  let(:group_merge_request_approval_setting) { build(:group_merge_request_approval_setting) }
+
+  subject { described_class.new(group_merge_request_approval_setting).as_json }
+
+  it 'exposes correct attributes' do
+    expect(subject.keys).to match(
+      [
+        :allow_author_approval,
+        :allow_committer_approval,
+        :allow_overrides_to_approver_list_per_merge_request,
+        :retain_approvals_on_push,
+        :require_password_to_approve
+      ]
+    )
+  end
+end

ee/spec/models/group_merge_request_approval_setting_spec.rb

@@ -15,6 +15,14 @@ RSpec.describe GroupMergeRequestApprovalSetting do
     it { is_expected.to validate_presence_of(:group) }
     it { is_expected.not_to allow_value(nil).for(:allow_author_approval) }
     it { is_expected.to allow_value(true, false).for(:allow_author_approval) }
+    it { is_expected.not_to allow_value(nil).for(:allow_committer_approval) }
+    it { is_expected.to allow_value(true, false).for(:allow_committer_approval) }
+    it { is_expected.not_to allow_value(nil).for(:allow_overrides_to_approver_list_per_merge_request) }
+    it { is_expected.to allow_value(true, false).for(:allow_overrides_to_approver_list_per_merge_request) }
+    it { is_expected.not_to allow_value(nil).for(:retain_approvals_on_push) }
+    it { is_expected.to allow_value(true, false).for(:retain_approvals_on_push) }
+    it { is_expected.not_to allow_value(nil).for(:require_password_to_approve) }
+    it { is_expected.to allow_value(true, false).for(:require_password_to_approve) }
   end
 
   describe '.find_or_initialize_by_group' do
@@ -22,11 +30,11 @@ RSpec.describe GroupMergeRequestApprovalSetting do
 
     subject { described_class.find_or_initialize_by_group(group) }
 
-    context 'no existing setting' do
+    context 'with no existing setting' do
       it { is_expected.to be_a_new_record }
     end
 
-    context 'existing setting' do
+    context 'with existing setting' do
       let_it_be(:setting) { create(:group_merge_request_approval_setting, group: group) }
 
       it { is_expected.to eq(setting) }

ee/spec/requests/api/group_merge_request_approval_settings_spec.rb

@@ -40,6 +40,10 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do
 
           expect(response).to have_gitlab_http_status(:ok)
           expect(json_response['allow_author_approval']).to eq(false)
+          expect(json_response['allow_committer_approval']).to eq(false)
+          expect(json_response['allow_overrides_to_approver_list_per_merge_request']).to eq(false)
+          expect(json_response['retain_approvals_on_push']).to eq(false)
+          expect(json_response['require_password_to_approve']).to eq(false)
         end
 
         it 'matches the response schema' do
@@ -129,6 +133,17 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do
             expect(json_response['message']).to eq('allow_author_approval' => ['must be a boolean value'])
           end
         end
+
+        context 'when invalid params' do
+          let(:params) { {} }
+
+          it 'returns 400 status with error message', :aggregate_failures do
+            put api(url, user), params: params
+
+            expect(response).to have_gitlab_http_status(:bad_request)
+            expect(json_response['error']).to match(/at least one parameter must be provided/)
+          end
+        end
       end
 
       context 'when the user is not authorised' do