Add more settings to group MR approval settings

This change adds the remaining settings to the group MR approval
settings table as well as exposing them via the existing API.

The naming of these settings are slightly different from the project
level counterparts. The rationale is to pick a name that enables the
`false` setting by default.

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

@@ -13265,7 +13265,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