Merge branch '332548-enforce-group-approval-settings' into 'master'

Enforce group approval settings in projects

See merge request gitlab-org/gitlab!69750

ee/app/assets/javascripts/approvals/components/approval_settings.vue

@@ -3,7 +3,8 @@ import { GlAlert, GlButton, GlForm, GlFormGroup, GlLoadingIcon, GlLink } from '@
 import { isEmpty } from 'lodash';
 import { mapActions, mapGetters, mapState } from 'vuex';
 import { helpPagePath } from '~/helpers/help_page_helper';
-import { APPROVAL_SETTINGS_I18N } from '../constants';
+import { sprintf } from '~/locale';
+import { APPROVAL_SETTINGS_I18N, TYPE_GROUP } from '../constants';
 import ApprovalSettingsCheckbox from './approval_settings_checkbox.vue';
 
 export default {
@@ -54,6 +55,7 @@ export default {
         state.approvalSettings.settings.preventMrApprovalRuleEdit,
       removeApprovalsOnPush: (state) => state.approvalSettings.settings.removeApprovalsOnPush,
       requireUserPassword: (state) => state.approvalSettings.settings.requireUserPassword,
+      groupName: (state) => state.settings.groupName,
     }),
     ...mapGetters(['settingChanged']),
     hasSettings() {
@@ -81,6 +83,16 @@ export default {
     async onSubmit() {
       await this.updateSettings(this.approvalSettingsPath);
     },
+    lockedText({ locked, inheritedFrom }) {
+      if (!locked) {
+        return null;
+      }
+      if (inheritedFrom === TYPE_GROUP) {
+        const { groupName } = this;
+        return sprintf(APPROVAL_SETTINGS_I18N.lockedByGroupOwner, { groupName });
+      }
+      return APPROVAL_SETTINGS_I18N.lockedByAdmin;
+    },
   },
   i18n: APPROVAL_SETTINGS_I18N,
   links: {
@@ -127,7 +139,7 @@ export default {
           :checked="preventAuthorApproval.value"
           :label="settingsLabels.authorApprovalLabel"
           :locked="!canPreventAuthorApproval || preventAuthorApproval.locked"
-          :locked-text="$options.i18n.lockedByAdmin"
+          :locked-text="lockedText(preventAuthorApproval)"
           data-testid="prevent-author-approval"
           @input="setPreventAuthorApproval"
         />
@@ -135,7 +147,7 @@ export default {
           :checked="preventCommittersApproval.value"
           :label="settingsLabels.preventCommittersApprovalLabel"
           :locked="!canPreventCommittersApproval || preventCommittersApproval.locked"
-          :locked-text="$options.i18n.lockedByAdmin"
+          :locked-text="lockedText(preventCommittersApproval)"
           data-testid="prevent-committers-approval"
           @input="setPreventCommittersApproval"
         />
@@ -143,7 +155,7 @@ export default {
           :checked="preventMrApprovalRuleEdit.value"
           :label="settingsLabels.preventMrApprovalRuleEditLabel"
           :locked="!canPreventMrApprovalRuleEdit || preventMrApprovalRuleEdit.locked"
-          :locked-text="$options.i18n.lockedByAdmin"
+          :locked-text="lockedText(preventMrApprovalRuleEdit)"
           data-testid="prevent-mr-approval-rule-edit"
           @input="setPreventMrApprovalRuleEdit"
         />
@@ -151,7 +163,7 @@ export default {
           :checked="requireUserPassword.value"
           :label="settingsLabels.requireUserPasswordLabel"
           :locked="requireUserPassword.locked"
-          :locked-text="$options.i18n.lockedByAdmin"
+          :locked-text="lockedText(requireUserPassword)"
           data-testid="require-user-password"
           @input="setRequireUserPassword"
         />
@@ -159,7 +171,7 @@ export default {
           :checked="removeApprovalsOnPush.value"
           :label="settingsLabels.removeApprovalsOnPushLabel"
           :locked="removeApprovalsOnPush.locked"
-          :locked-text="$options.i18n.lockedByAdmin"
+          :locked-text="lockedText(removeApprovalsOnPush)"
           data-testid="remove-approvals-on-push"
           @input="setRemoveApprovalsOnPush"
         />

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

@@ -64,6 +64,9 @@ export const APPROVAL_SETTINGS_I18N = {
   lockedByAdmin: s__(
     'ApprovalSettings|This setting is configured at the instance level and can only be changed by an administrator.',
   ),
+  lockedByGroupOwner: s__(
+    'ApprovalSettings|This setting is configured in %{groupName} and can only be changed by an administrator or group owner.',
+  ),
 };
 
 export const PROJECT_APPROVAL_SETTINGS_LABELS_I18N = {

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

+import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
 import { RULE_TYPE_REGULAR, RULE_TYPE_ANY_APPROVER } from './constants';
 
 const visibleTypes = new Set([RULE_TYPE_ANY_APPROVER, RULE_TYPE_REGULAR]);
@@ -102,8 +103,10 @@ export const mapMRApprovalSettingsResponse = (res) => {
 
 const invertApprovalSetting = ({ value, ...rest }) => ({ value: !value, ...rest });
 
-export const groupApprovalsMappers = {
-  mapDataToState: (data) => ({
+export const mergeRequestApprovalSettingsMappers = {
+  mapDataToState: (data) =>
+    convertObjectPropsToCamelCase(
+      {
         preventAuthorApproval: invertApprovalSetting(data.allow_author_approval),
         preventMrApprovalRuleEdit: invertApprovalSetting(
           data.allow_overrides_to_approver_list_per_merge_request,
@@ -111,7 +114,9 @@ export const groupApprovalsMappers = {
         requireUserPassword: data.require_password_to_approve,
         removeApprovalsOnPush: invertApprovalSetting(data.retain_approvals_on_push),
         preventCommittersApproval: invertApprovalSetting(data.allow_committer_approval),
-  }),
+      },
+      { deep: true },
+    ),
   mapStateToPayload: ({ settings }) => ({
     allow_author_approval: !settings.preventAuthorApproval.value,
     allow_overrides_to_approver_list_per_merge_request: !settings.preventMrApprovalRuleEdit.value,

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

 import Vue from 'vue';
 import { parseBoolean } from '~/lib/utils/common_utils';
 import GroupSettingsApp from './components/group_settings/app.vue';
-import { groupApprovalsMappers } from './mappers';
+import { mergeRequestApprovalSettingsMappers } from './mappers';
 import createStore from './stores';
 import approvalSettingsModule from './stores/modules/approval_settings';
 
@@ -12,7 +12,7 @@ const mountGroupApprovalSettings = (el) => {
 
   const { defaultExpanded, approvalSettingsPath } = el.dataset;
   const store = createStore({
-    approvalSettings: approvalSettingsModule(groupApprovalsMappers),
+    approvalSettings: approvalSettingsModule(mergeRequestApprovalSettingsMappers),
   });
 
   return new Vue({

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

 import Vue from 'vue';
 import { parseBoolean } from '~/lib/utils/common_utils';
 import ProjectSettingsApp from './components/project_settings/app.vue';
-import { projectApprovalsMappers } from './mappers';
+import { projectApprovalsMappers, mergeRequestApprovalSettingsMappers } from './mappers';
 import createStore from './stores';
 import approvalSettingsModule from './stores/modules/approval_settings';
 import projectSettingsModule from './stores/modules/project_settings';
@@ -18,10 +18,18 @@ export default function mountProjectSettingsApprovals(el) {
   } = el.dataset;
 
   const modules = {
-    approvalSettings: approvalSettingsModule({ updateMethod: 'post', ...projectApprovalsMappers }),
     approvals: projectSettingsModule(),
   };
 
+  if (gon.features.groupMergeRequestApprovalSettingsFeatureFlag) {
+    modules.approvalSettings = approvalSettingsModule(mergeRequestApprovalSettingsMappers);
+  } else {
+    modules.approvalSettings = approvalSettingsModule({
+      updateMethod: 'post',
+      ...projectApprovalsMappers,
+    });
+  }
+
   const store = createStore(modules, {
     ...el.dataset,
     prefix: 'project-settings',

ee/app/assets/javascripts/approvals/stores/modules/approval_settings/mutations.js

@@ -8,7 +8,7 @@ export default (mapDataToState) => ({
     state.errorMessage = '';
   },
   [types.RECEIVE_SETTINGS_SUCCESS](state, data) {
-    state.settings = { ...mapDataToState(data) };
+    state.settings = mapDataToState(data);
     state.initialSettings = cloneDeep(state.settings);
     state.isLoading = false;
   },
@@ -22,7 +22,7 @@ export default (mapDataToState) => ({
     state.errorMessage = '';
   },
   [types.UPDATE_SETTINGS_SUCCESS](state, data) {
-    state.settings = { ...mapDataToState(data) };
+    state.settings = mapDataToState(data);
     state.initialSettings = cloneDeep(state.settings);
     state.isLoading = false;
     state.isUpdated = true;

ee/app/controllers/ee/projects_controller.rb

@@ -14,6 +14,10 @@ module EE
         enable_sast_entry_points_experiment
       end
 
+      before_action only: :edit do
+        push_frontend_feature_flag(:group_merge_request_approval_settings_feature_flag, project.root_ancestor)
+      end
+
       feature_category :projects, [:restore]
     end
 

ee/app/helpers/ee/projects_helper.rb

@@ -59,24 +59,27 @@ module EE
 
     def approvals_app_data(project = @project)
       {
-        data: {
-          'project_id': project.id,
-          'can_edit': can_modify_approvers.to_s,
-          'can_modify_author_settings': can_modify_author_settings.to_s,
-          'can_modify_commiter_settings': can_modify_commiter_settings.to_s,
-          'project_path': expose_path(api_v4_projects_path(id: project.id)),
-          'settings_path': expose_path(api_v4_projects_approval_settings_path(id: project.id)),
-          'approvals_path': expose_path(api_v4_projects_approvals_path(id: project.id)),
-          'rules_path': expose_path(api_v4_projects_approval_settings_rules_path(id: project.id)),
-          'allow_multi_rule': project.multiple_approval_rules_available?.to_s,
-          'eligible_approvers_docs_path': help_page_path('user/project/merge_requests/approvals/rules', anchor: 'eligible-approvers'),
-          'security_approvals_help_page_path': help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
-          'security_configuration_path': project_security_configuration_path(project),
-          'vulnerability_check_help_page_path': help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
-          'license_check_help_page_path': help_page_path('user/application_security/index', anchor: 'enabling-license-approvals-within-a-project'),
-          'coverage_check_help_page_path': help_page_path('ci/pipelines/settings', anchor: 'coverage-check-approval-rule')
-        }
-      }
+        project_id: project.id,
+        can_edit: can_modify_approvers.to_s,
+        can_modify_author_settings: can_modify_author_settings.to_s,
+        can_modify_commiter_settings: can_modify_commiter_settings.to_s,
+        project_path: expose_path(api_v4_projects_path(id: project.id)),
+        settings_path: expose_path(api_v4_projects_approval_settings_path(id: project.id)),
+        approvals_path: expose_path(api_v4_projects_approvals_path(id: project.id)),
+        rules_path: expose_path(api_v4_projects_approval_settings_rules_path(id: project.id)),
+        allow_multi_rule: project.multiple_approval_rules_available?.to_s,
+        eligible_approvers_docs_path: help_page_path('user/project/merge_requests/approvals/rules', anchor: 'eligible-approvers'),
+        security_approvals_help_page_path: help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
+        security_configuration_path: project_security_configuration_path(project),
+        vulnerability_check_help_page_path: help_page_path('user/application_security/index', anchor: 'security-approvals-in-merge-requests'),
+        license_check_help_page_path: help_page_path('user/application_security/index', anchor: 'enabling-license-approvals-within-a-project'),
+        coverage_check_help_page_path: help_page_path('ci/pipelines/settings', anchor: 'coverage-check-approval-rule')
+      }.tap do |data|
+        if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, project.root_ancestor)
+          data[:approvals_path] = expose_path(api_v4_projects_merge_request_approval_setting_path(id: project.id))
+          data[:group_name] = project.root_ancestor.name
+        end
+      end
     end
 
     def status_checks_app_data(project)

ee/app/views/projects/_merge_request_approvals_settings_form.html.haml

 .form-group
   = label :approver_ids, class: 'label-bold' do
     = _("Approval rules")
-  #js-mr-approvals-settings{ approvals_app_data }
+  #js-mr-approvals-settings{ data: approvals_app_data }
     .text-center.gl-mt-3
       = sprite_icon('spinner', size: 24, css_class: 'gl-spinner')

ee/spec/features/projects/audit_events_spec.rb

@@ -133,8 +133,14 @@ RSpec.describe 'Projects > Audit Events', :js do
     end
   end
 
+  context 'with the group_merge_request_approval_settings feature' do
+    where(feature_enabled: [true, false])
+
+    with_them do
       describe 'changing merge request approval permission for authors and reviewers' do
         before do
+          stub_feature_flags(group_merge_request_approval_settings_feature_flag: feature_enabled)
+          stub_licensed_features(group_merge_request_approval_settings: feature_enabled)
           project.add_developer(pete)
         end
 
@@ -164,6 +170,8 @@ RSpec.describe 'Projects > Audit Events', :js do
           end
         end
       end
+    end
+  end
 
   describe 'filter by date' do
     let!(:audit_event_1) { create(:project_audit_event, entity_type: 'Project', entity_id: project.id, created_at: 5.days.ago) }

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

@@ -7,11 +7,13 @@ import {
   PROJECT_APPROVAL_SETTINGS_LABELS_I18N,
   APPROVAL_SETTINGS_I18N,
 } from 'ee/approvals/constants';
-import { groupApprovalsMappers } from 'ee/approvals/mappers';
+import { mergeRequestApprovalSettingsMappers } from 'ee/approvals/mappers';
 import createStore from 'ee/approvals/stores';
 import approvalSettingsModule from 'ee/approvals/stores/modules/approval_settings/';
+import projectSettingsModule from 'ee/approvals/stores/modules/project_settings/';
 import { extendedWrapper } from 'helpers/vue_test_utils_helper';
 import waitForPromises from 'helpers/wait_for_promises';
+import { sprintf } from '~/locale';
 import { createGroupApprovalsPayload, createGroupApprovalsState } from '../mocks';
 
 const localVue = createLocalVue();
@@ -26,7 +28,7 @@ describe('ApprovalSettings', () => {
   const approvalSettingsPath = 'groups/22/merge_request_approval_settings';
 
   const setupStore = (data = {}, initialData) => {
-    const module = approvalSettingsModule(groupApprovalsMappers);
+    const module = approvalSettingsModule(mergeRequestApprovalSettingsMappers);
 
     module.state.settings = data;
     module.state.initialSettings = initialData || data;
@@ -36,7 +38,7 @@ describe('ApprovalSettings', () => {
     jest.spyOn(actions, 'dismissErrorMessage').mockImplementation();
     jest.spyOn(actions, 'dismissSuccessMessage').mockImplementation();
 
-    store = createStore({ approvalSettings: module });
+    store = createStore({ approvalSettings: module, approvals: projectSettingsModule() });
   };
 
   const createWrapper = (props = {}) => {
@@ -185,8 +187,10 @@ describe('ApprovalSettings', () => {
       ${'remove-approvals-on-push'}      | ${'setRemoveApprovalsOnPush'}     | ${'removeApprovalsOnPush'}     | ${'removeApprovalsOnPushLabel'}
     `('with the $testid checkbox', ({ testid, action, setting, labelKey }) => {
       let checkbox = null;
+      const groupName = 'GitLab Org';
 
       beforeEach(async () => {
+        store.state.settings.groupName = groupName;
         jest.spyOn(store, 'dispatch').mockImplementation();
         createWrapper();
         await waitForPromises();
@@ -205,11 +209,22 @@ describe('ApprovalSettings', () => {
         expect(checkbox.props('label')).toBe(PROJECT_APPROVAL_SETTINGS_LABELS_I18N[labelKey]);
       });
 
-      it('sets the locked and lockedText based on the setting values', () => {
-        expect(checkbox.props()).toMatchObject({
-          locked: settings[setting].locked,
-          lockedText: APPROVAL_SETTINGS_I18N.lockedByAdmin,
+      it('sets the locked prop', () => {
+        expect(checkbox.props('locked')).toBe(settings[setting].locked);
       });
+
+      it('sets the lockedText prop', () => {
+        const { inheritedFrom, locked } = settings[setting];
+
+        let expectedText = null;
+
+        if (locked && inheritedFrom === 'group') {
+          expectedText = sprintf(APPROVAL_SETTINGS_I18N.lockedByGroupOwner, { groupName });
+        } else if (locked && inheritedFrom === 'instance') {
+          expectedText = APPROVAL_SETTINGS_I18N.lockedByAdmin;
+        }
+
+        expect(checkbox.props('lockedText')).toBe(expectedText);
       });
 
       it(`triggers the action ${action} when the value is changed`, async () => {
@@ -273,13 +288,7 @@ describe('ApprovalSettings', () => {
 
     describe('locked settings', () => {
       beforeEach(() => {
-        setupStore({
-          ...settings,
-          preventAuthorApproval: {
-            ...settings.preventAuthorApproval,
-            locked: false,
-          },
-        });
+        setupStore(createGroupApprovalsState(false).settings);
       });
 
       it.each`

ee/spec/frontend/approvals/components/group_settings/app_spec.js

@@ -7,7 +7,7 @@ import Vuex from 'vuex';
 import ApprovalSettings from 'ee/approvals/components/approval_settings.vue';
 import GroupSettingsApp from 'ee/approvals/components/group_settings/app.vue';
 import { GROUP_APPROVAL_SETTINGS_LABELS_I18N } from 'ee/approvals/constants';
-import { groupApprovalsMappers } from 'ee/approvals/mappers';
+import { mergeRequestApprovalSettingsMappers } from 'ee/approvals/mappers';
 import { createStoreOptions } from 'ee/approvals/stores';
 import approvalSettingsModule from 'ee/approvals/stores/modules/approval_settings';
 import SettingsBlock from '~/vue_shared/components/settings/settings_block.vue';
@@ -44,7 +44,9 @@ describe('EE Approvals Group Settings App', () => {
     axiosMock = new MockAdapter(axios);
     axiosMock.onGet('*');
 
-    store = createStoreOptions({ approvalSettings: approvalSettingsModule(groupApprovalsMappers) });
+    store = createStoreOptions({
+      approvalSettings: approvalSettingsModule(mergeRequestApprovalSettingsMappers),
+    });
   });
 
   afterEach(() => {

ee/spec/frontend/approvals/mappers_spec.js

-import { groupApprovalsMappers } from 'ee/approvals/mappers';
+import { mergeRequestApprovalSettingsMappers } from 'ee/approvals/mappers';
 import { createGroupApprovalsPayload, createGroupApprovalsState } from './mocks';
 
 describe('approvals mappers', () => {
-  describe('groupApprovalsMappers', () => {
+  describe('mergeRequestApprovalSettingsMappers', () => {
     const approvalsState = createGroupApprovalsState();
     const approvalsFetchPayload = createGroupApprovalsPayload();
     const approvalsUpdatePayload = {
@@ -14,13 +14,13 @@ describe('approvals mappers', () => {
     };
 
     it('maps data to state', () => {
-      expect(groupApprovalsMappers.mapDataToState(approvalsFetchPayload)).toStrictEqual(
-        approvalsState.settings,
-      );
+      expect(
+        mergeRequestApprovalSettingsMappers.mapDataToState(approvalsFetchPayload),
+      ).toStrictEqual(approvalsState.settings);
     });
 
     it('maps state to payload', () => {
-      expect(groupApprovalsMappers.mapStateToPayload(approvalsState)).toStrictEqual(
+      expect(mergeRequestApprovalSettingsMappers.mapStateToPayload(approvalsState)).toStrictEqual(
         approvalsUpdatePayload,
       );
     });

ee/spec/frontend/approvals/mocks.js

@@ -44,8 +44,8 @@ export const createGroupApprovalsPayload = () => ({
   },
   allow_committer_approval: {
     value: true,
-    locked: false,
-    inherited_from: null,
+    locked: true,
+    inherited_from: 'group',
   },
   allow_overrides_to_approver_list_per_merge_request: {
     value: true,
@@ -64,31 +64,31 @@ export const createGroupApprovalsPayload = () => ({
   },
 });
 
-export const createGroupApprovalsState = () => ({
+export const createGroupApprovalsState = (locked = null) => ({
   settings: {
     preventAuthorApproval: {
-      inherited_from: 'instance',
-      locked: true,
+      inheritedFrom: 'instance',
+      locked: locked ?? true,
       value: false,
     },
     preventCommittersApproval: {
-      inherited_from: null,
-      locked: false,
+      inheritedFrom: 'group',
+      locked: locked ?? true,
       value: false,
     },
     preventMrApprovalRuleEdit: {
-      inherited_from: null,
-      locked: false,
+      inheritedFrom: null,
+      locked: locked ?? false,
       value: false,
     },
     removeApprovalsOnPush: {
-      inherited_from: null,
-      locked: null,
+      inheritedFrom: null,
+      locked,
       value: false,
     },
     requireUserPassword: {
-      inherited_from: null,
-      locked: null,
+      inheritedFrom: null,
+      locked,
       value: true,
     },
   },

ee/spec/helpers/projects_helper_spec.rb

@@ -373,8 +373,13 @@ RSpec.describe ProjectsHelper do
       allow(helper).to receive(:can?).and_return(true)
     end
 
+    context 'with group_merge_request_approval_settings_feature_flag disabled' do
+      before do
+        stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
+      end
+
       it 'returns the correct data' do
-      expect(subject[:data]).to eq({
+        expect(subject).to eq({
           project_id: project.id,
           can_edit: 'true',
           can_modify_author_settings: 'true',
@@ -394,6 +399,20 @@ RSpec.describe ProjectsHelper do
       end
     end
 
+    context 'with group_merge_request_approval_settings_feature_flag enabled' do
+      before do
+        stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
+      end
+
+      it 'returns the correct data' do
+        expect(subject).to include(
+          approvals_path: expose_path(api_v4_projects_merge_request_approval_setting_path(id: project.id)),
+          group_name: project.root_ancestor.name
+        )
+      end
+    end
+  end
+
   describe '#status_checks_app_data' do
     subject { helper.status_checks_app_data(project) }
 

locale/gitlab.pot

@@ -4276,6 +4276,9 @@ msgstr ""
 msgid "ApprovalSettings|This setting is configured at the instance level and can only be changed by an administrator."
 msgstr ""
 
+msgid "ApprovalSettings|This setting is configured in %{groupName} and can only be changed by an administrator or group owner."
+msgstr ""
+
 msgid "ApprovalStatusTooltip|Adheres to separation of duties"
 msgstr ""