Merge branch '325279-group-level-mr' into 'master'

Integrate instance-level with group-level MR approval settings

See merge request gitlab-org/gitlab!67316

ee/app/assets/javascripts/approvals/mappers.js

@@ -100,11 +100,11 @@ export const mapMRApprovalSettingsResponse = (res) => {
 
 export const groupApprovalsMappers = {
   mapDataToState: (data) => ({
-    preventAuthorApproval: !data.allow_author_approval,
-    preventMrApprovalRuleEdit: !data.allow_overrides_to_approver_list_per_merge_request,
-    requireUserPassword: data.require_password_to_approve,
-    removeApprovalsOnPush: !data.retain_approvals_on_push,
-    preventCommittersApproval: !data.allow_committer_approval,
+    preventAuthorApproval: !data.allow_author_approval.value,
+    preventMrApprovalRuleEdit: !data.allow_overrides_to_approver_list_per_merge_request.value,
+    requireUserPassword: data.require_password_to_approve.value,
+    removeApprovalsOnPush: !data.retain_approvals_on_push.value,
+    preventCommittersApproval: !data.allow_committer_approval.value,
   }),
   mapStateToPayload: (state) => ({
     allow_author_approval: !state.settings.preventAuthorApproval,

ee/lib/api/group_merge_request_approval_settings.rb

@@ -21,7 +21,7 @@ module API
           success ::API::Entities::GroupMergeRequestApprovalSetting
         end
         get do
-          setting = GroupMergeRequestApprovalSetting.find_or_initialize_by_group(user_group)
+          setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(user_group).execute
 
           present setting, with: ::API::Entities::GroupMergeRequestApprovalSetting
         end
@@ -52,7 +52,9 @@ module API
             .new(container: user_group, current_user: current_user, params: setting_params).execute
 
           if response.success?
-            present response.payload, with: ::API::Entities::GroupMergeRequestApprovalSetting
+            setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(user_group).execute
+
+            present setting, with: ::API::Entities::GroupMergeRequestApprovalSetting
           else
             render_api_error!(response.message, :bad_request)
           end

ee/lib/compliance_management/merge_request_approval_settings/resolver.rb

+# frozen_string_literal: true
+module ComplianceManagement
+  module MergeRequestApprovalSettings
+    class Resolver
+      def initialize(group)
+        @group = group
+      end
+
+      def execute
+        {
+          allow_author_approval: allow_author_approval,
+          allow_committer_approval: allow_committer_approval,
+          allow_overrides_to_approver_list_per_merge_request: allow_overrides_to_approver_list_per_merge_request,
+          retain_approvals_on_push: retain_approvals_on_push,
+          require_password_to_approve: require_password_to_approve
+        }
+      end
+
+      def allow_author_approval
+        instance_value = instance_settings.current_application_settings.prevent_merge_requests_author_approval
+        group_value = group_settings&.allow_author_approval
+
+        setting(:allow_author_approval, instance_value, group_value, default: true)
+      end
+
+      def allow_committer_approval
+        instance_value = instance_settings.prevent_merge_requests_committers_approval
+        group_value = group_settings&.allow_committer_approval
+
+        setting(:allow_committer_approval, instance_value, group_value, default: true)
+      end
+
+      def allow_overrides_to_approver_list_per_merge_request
+        instance_value = instance_settings.disable_overriding_approvers_per_merge_request
+        group_value = group_settings&.allow_overrides_to_approver_list_per_merge_request
+
+        setting(:allow_overrides_to_approver_list_per_merge_request, instance_value, group_value, default: true)
+      end
+
+      def retain_approvals_on_push
+        instance_value = nil
+        group_value = group_settings&.retain_approvals_on_push
+
+        setting(:retain_approvals_on_push, instance_value, group_value, default: true)
+      end
+
+      def require_password_to_approve
+        instance_value = nil
+        group_value = group_settings&.require_password_to_approve
+
+        setting(:require_password_to_approve, instance_value, group_value, default: false)
+      end
+
+      private
+
+      def instance_settings
+        ::Gitlab::CurrentSettings
+      end
+
+      def group_settings
+        @group&.group_merge_request_approval_setting
+      end
+
+      def setting(name, instance_value, group_value, default:)
+        # Setting value should be the instance-level, if set.
+        # Otherwise group-level.
+        # Finally, fallback on to the default defined here.
+        value = instance_value ? !instance_value : group_value
+
+        ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
+          value: value.nil? ? default : value,
+          locked: instance_value.nil? ? false : instance_value,
+          inherited_from: instance_value ? :instance : nil
+        )
+      end
+    end
+  end
+end

ee/lib/compliance_management/merge_request_approval_settings/setting.rb

+# frozen_string_literal: true
+
+module ComplianceManagement
+  module MergeRequestApprovalSettings
+    class Setting
+      attr_reader :value, :locked, :inherited_from
+
+      def initialize(value:, locked:, inherited_from:)
+        @value = value
+        @locked = locked
+        @inherited_from = inherited_from
+      end
+    end
+  end
+end

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

 {
   "type": "object",
   "properties": {
-    "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" }
+    "allow_author_approval": {
+      "value": { "type":  "boolean" },
+      "locked": { "type":  "boolean" },
+      "inherited_from": { "type":  "string" }
+    },
+    "allow_committer_approval": {
+      "value": { "type":  "boolean" },
+      "locked": { "type":  "boolean" },
+      "inherited_from": { "type":  "string" }
+    },
+    "allow_overrides_to_approver_list_per_merge_request": {
+      "value": { "type":  "boolean" },
+      "locked": { "type":  "boolean" },
+      "inherited_from": { "type":  "string" }
+    },
+    "retain_approvals_on_push": {
+      "value": { "type":  "boolean" },
+      "locked": { "type":  "boolean" },
+      "inherited_from": { "type":  "string" }
+    },
+    "require_password_to_approve": {
+      "value": { "type":  "boolean" },
+      "locked": { "type":  "boolean" },
+      "inherited_from": { "type":  "string" }
+    }
   },
   "additionalProperties": false
 }

ee/spec/frontend/approvals/components/approval_settings_spec.js

@@ -9,6 +9,7 @@ import createStore from 'ee/approvals/stores';
 import approvalSettingsModule from 'ee/approvals/stores/modules/approval_settings/';
 import { extendedWrapper } from 'helpers/vue_test_utils_helper';
 import waitForPromises from 'helpers/wait_for_promises';
+import { createGroupApprovalsPayload } from '../mocks';
 
 const localVue = createLocalVue();
 localVue.use(Vuex);
@@ -18,6 +19,7 @@ describe('ApprovalSettings', () => {
   let store;
   let actions;
 
+  const groupApprovalsPayload = createGroupApprovalsPayload();
   const approvalSettingsPath = 'groups/22/merge_request_approval_settings';
 
   const setupStore = (data = {}, initialData) => {
@@ -215,7 +217,7 @@ describe('ApprovalSettings', () => {
 
           await waitForPromises();
           await findForm().vm.$emit('submit', { preventDefault: () => {} });
-          await store.commit('UPDATE_SETTINGS_SUCCESS', {});
+          await store.commit('UPDATE_SETTINGS_SUCCESS', groupApprovalsPayload);
         });
 
         it('update the settings', () => {

ee/spec/frontend/approvals/mappers_spec.js

 import { groupApprovalsMappers } from 'ee/approvals/mappers';
+import { createGroupApprovalsPayload } from './mocks';
 
 describe('approvals mappers', () => {
   describe('groupApprovalsMappers', () => {
-    const groupPayload = {
+    const groupFetchPayload = createGroupApprovalsPayload();
+    const groupUpdatePayload = {
       allow_author_approval: true,
       allow_overrides_to_approver_list_per_merge_request: true,
       require_password_to_approve: true,
@@ -20,11 +22,13 @@ describe('approvals mappers', () => {
     };
 
     it('maps data to state', () => {
-      expect(groupApprovalsMappers.mapDataToState(groupPayload)).toStrictEqual(groupState.settings);
+      expect(groupApprovalsMappers.mapDataToState(groupFetchPayload)).toStrictEqual(
+        groupState.settings,
+      );
     });
 
     it('maps state to payload', () => {
-      expect(groupApprovalsMappers.mapStateToPayload(groupState)).toStrictEqual(groupPayload);
+      expect(groupApprovalsMappers.mapStateToPayload(groupState)).toStrictEqual(groupUpdatePayload);
     });
   });
 });

ee/spec/frontend/approvals/mocks.js

@@ -35,3 +35,31 @@ export const createMRRuleWithSource = () => ({
   hasSource: true,
   sourceId: 3,
 });
+
+export const createGroupApprovalsPayload = () => ({
+  allow_author_approval: {
+    value: true,
+    locked: true,
+    inherited_from: 'instance',
+  },
+  allow_committer_approval: {
+    value: true,
+    locked: false,
+    inherited_from: null,
+  },
+  allow_overrides_to_approver_list_per_merge_request: {
+    value: true,
+    locked: false,
+    inherited_from: null,
+  },
+  retain_approvals_on_push: {
+    value: true,
+    locked: null,
+    inherited_from: null,
+  },
+  require_password_to_approve: {
+    value: true,
+    locked: null,
+    inherited_from: null,
+  },
+});

ee/spec/lib/compliance_management/merge_request_approval_settings/resolver_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::ComplianceManagement::MergeRequestApprovalSettings::Resolver do
+  using RSpec::Parameterized::TableSyntax
+
+  let_it_be(:group) { create(:group) }
+
+  before(:all) do
+    group.create_group_merge_request_approval_setting
+  end
+
+  it 'is initialized' do
+    resolver = described_class.new(group)
+
+    expect(resolver).not_to be_nil
+  end
+
+  shared_examples 'resolvable but has no instance setting' do
+    where(:group_allows_approval, :value, :locked, :inherited_from) do
+      true  | true  | false | nil
+      false | false | false | nil
+    end
+
+    with_them do
+      before do
+        group.group_merge_request_approval_setting.update!(instance_method => group_allows_approval)
+      end
+
+      let(:object) { described_class.new(group).send(instance_method) }
+
+      it 'has the correct value' do
+        expect(object.value).to eq(value)
+      end
+
+      it 'has the correct locked status' do
+        expect(object.locked).to eq(locked)
+      end
+
+      it 'has the correct inheritance' do
+        expect(object.inherited_from).to eq(inherited_from)
+      end
+    end
+  end
+
+  shared_examples 'resolvable' do
+    where(:instance_prevents_approval, :group_allows_approval, :value, :locked, :inherited_from) do
+      true  | true  | false | true  | :instance
+      true  | false | false | true  | :instance
+      false | true  | true  | false | nil
+      false | false | false | false | nil
+    end
+
+    with_them do
+      before do
+        stub_ee_application_setting(instance_flag => instance_prevents_approval)
+        group.group_merge_request_approval_setting.update!(group_flag => group_allows_approval)
+      end
+
+      let(:object) { described_class.new(group).send(group_flag) }
+
+      it 'has the correct value' do
+        expect(object.value).to eq(value)
+      end
+
+      it 'has the correct locked status' do
+        expect(object.locked).to eq(locked)
+      end
+
+      it 'has the correct inheritance' do
+        expect(object.inherited_from).to eq(inherited_from)
+      end
+    end
+  end
+
+  describe '#allow_author_approval' do
+    let(:instance_flag) { :prevent_merge_requests_author_approval }
+    let(:group_flag) { :allow_author_approval }
+
+    it_behaves_like 'resolvable'
+  end
+
+  describe '#allow_committer_approval' do
+    let(:instance_flag) { :prevent_merge_requests_committers_approval }
+    let(:group_flag) { :allow_committer_approval }
+
+    it_behaves_like 'resolvable'
+  end
+
+  describe '#allow_overrides_to_approver_list_per_merge_request' do
+    let(:instance_flag) { :disable_overriding_approvers_per_merge_request }
+    let(:group_flag) { :allow_overrides_to_approver_list_per_merge_request }
+
+    it_behaves_like 'resolvable'
+  end
+
+  describe '#retain_approvals_on_push' do
+    let(:instance_method) { :retain_approvals_on_push }
+
+    it_behaves_like 'resolvable but has no instance setting'
+  end
+
+  describe '#require_password_to_approve' do
+    let(:instance_method) { :require_password_to_approve }
+
+    it_behaves_like 'resolvable but has no instance setting'
+  end
+end

ee/spec/requests/api/group_merge_request_approval_settings_spec.rb

@@ -9,6 +9,40 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do
 
   let(:url) { "/groups/#{group.id}/merge_request_approval_setting" }
 
+  shared_examples "resolvable" do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:instance_prevents_approval, :group_allows_approval, :value, :locked, :inherited_from) do
+      true  | true  | false | true  | 'instance'
+      true  | false | false | true  | 'instance'
+      false | true  | true  | false | nil
+      false | false | false | false | nil
+    end
+
+    with_them do
+      before do
+        stub_ee_application_setting(instance_flag => instance_prevents_approval)
+        group.group_merge_request_approval_setting.update!(group_flag => group_allows_approval)
+
+        get api(url, user)
+      end
+
+      let(:object) { json_response[group_flag.to_s] }
+
+      it 'has the correct value' do
+        expect(object['value']).to eq(value)
+      end
+
+      it 'has the correct locked status' do
+        expect(object['locked']).to eq(locked)
+      end
+
+      it 'has the correct inheritance' do
+        expect(object['inherited_from']).to eq(inherited_from)
+      end
+    end
+  end
+
   describe 'GET /groups/:id/merge_request_approval_settings' do
     context 'when feature flag is disabled' do
       before do
@@ -35,15 +69,25 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do
             .and_return(true)
         end
 
-        it 'returns 200 status with correct response body', :aggregate_failures do
-          get api(url, user)
+        context 'allow_author_approval values' do
+          let(:instance_flag) { :prevent_merge_requests_author_approval }
+          let(:group_flag) { :allow_author_approval }
 
-          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)
+          it_behaves_like 'resolvable'
+        end
+
+        context 'allow_committer_approval values' do
+          let(:instance_flag) { :prevent_merge_requests_committers_approval }
+          let(:group_flag) { :allow_committer_approval }
+
+          it_behaves_like 'resolvable'
+        end
+
+        context 'allow_overrides_to_approver_list_per_merge_request values' do
+          let(:instance_flag) { :disable_overriding_approvers_per_merge_request }
+          let(:group_flag) { :allow_overrides_to_approver_list_per_merge_request }
+
+          it_behaves_like 'resolvable'
         end
 
         it 'matches the response schema' do
@@ -61,7 +105,11 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do
             get api(url, user)
 
             expect(response).to have_gitlab_http_status(:ok)
-            expect(json_response['allow_author_approval']).to eq(false)
+            expect(json_response['allow_author_approval']['value']).to eq(true)
+            expect(json_response['allow_committer_approval']['value']).to eq(true)
+            expect(json_response['allow_overrides_to_approver_list_per_merge_request']['value']).to eq(true)
+            expect(json_response['retain_approvals_on_push']['value']).to eq(true)
+            expect(json_response['require_password_to_approve']['value']).to eq(false)
           end
         end
       end
@@ -114,7 +162,7 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do
           put api(url, user), params: params
 
           expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response['allow_author_approval']).to eq(true)
+          expect(json_response['allow_author_approval']['value']).to eq(true)
         end
 
         it 'matches the response schema' do