Allow users to save group MR approval settings

This commit adds saving capability to the update MR approval settings
form on Group settings page. The form submission uses the Group MR
approval settings RESTful API.

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

@@ -21,7 +21,7 @@ export default {
   },
   computed: {
     ...mapState({
-      preventAuthorApproval: (state) => state.approvals.preventAuthorApproval,
+      settings: (state) => state.approvals.settings,
       isLoading: (state) => state.approvals.isLoading,
     }),
   },
@@ -29,7 +29,10 @@ export default {
     this.fetchSettings(this.approvalSettingsPath);
   },
   methods: {
-    ...mapActions(['fetchSettings', 'updatePreventAuthorApproval']),
+    ...mapActions(['fetchSettings', 'updateSettings']),
+    onSubmit() {
+      this.updateSettings(this.approvalSettingsPath);
+    },
   },
   links: {
     preventAuthorApprovalDocsPath: helpPagePath(
@@ -48,12 +51,11 @@ export default {
 </script>
 
 <template>
-  <gl-form>
+  <gl-form @submit.prevent="onSubmit">
     <gl-form-group>
       <gl-form-checkbox
-        :checked="preventAuthorApproval"
+        v-model="settings.preventAuthorApproval"
         data-testid="prevent-author-approval"
-        @input="updatePreventAuthorApproval"
       >
         {{ $options.i18n.authorApprovalLabel }}
         <gl-link :href="$options.links.preventAuthorApprovalDocsPath" target="_blank">

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

@@ -34,7 +34,7 @@ export default {
 </script>
 
 <template>
-  <settings-block :default-expanded="defaultExpanded">
+  <settings-block :default-expanded="defaultExpanded" data-testid="merge-request-approval-settings">
     <template #title> {{ $options.i18n.groupSettingsHeader }}</template>
     <template #description>
       <gl-sprintf :message="$options.i18n.groupSettingsDescription">

ee/app/assets/javascripts/approvals/stores/modules/group_settings/actions.js

@@ -23,6 +23,26 @@ export const fetchSettings = ({ commit }, endpoint) => {
     });
 };
 
-export const updatePreventAuthorApproval = ({ commit }, preventAuthorApproval) => {
-  commit(types.UPDATE_PREVENT_AUTHOR_APPROVAL, preventAuthorApproval);
+export const updateSettings = ({ commit, state }, endpoint) => {
+  const payload = {
+    allow_author_approval: !state.settings.preventAuthorApproval,
+  };
+
+  commit(types.REQUEST_UPDATE_SETTINGS);
+
+  return axios
+    .put(endpoint, payload)
+    .then(({ data }) => {
+      commit(types.UPDATE_SETTINGS_SUCCESS, data);
+    })
+    .catch(({ response }) => {
+      const error = response?.data?.message;
+
+      commit(types.UPDATE_SETTINGS_ERROR, error);
+      createFlash({
+        message: __('There was an error updating merge request approval settings.'),
+        captureError: true,
+        error,
+      });
+    });
 };

ee/app/assets/javascripts/approvals/stores/modules/group_settings/mutation_types.js

@@ -3,4 +3,6 @@ export * from '../base/mutation_types';
 export const REQUEST_SETTINGS = 'REQUEST_SETTINGS';
 export const RECEIVE_SETTINGS_SUCCESS = 'RECEIVE_SETTINGS_SUCCESS';
 export const RECEIVE_SETTINGS_ERROR = 'RECEIVE_SETTINGS_ERROR';
-export const UPDATE_PREVENT_AUTHOR_APPROVAL = 'UPDATE_PREVENT_AUTHOR_APPROVAL';
+export const REQUEST_UPDATE_SETTINGS = 'REQUEST_UPDATE_SETTINGS';
+export const UPDATE_SETTINGS_SUCCESS = 'UPDATE_SETTINGS_SUCCESS';
+export const UPDATE_SETTINGS_ERROR = 'UPDATE_SETTINGS_ERROR';

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

@@ -5,13 +5,20 @@ export default {
     state.isLoading = true;
   },
   [types.RECEIVE_SETTINGS_SUCCESS](state, data) {
-    state.preventAuthorApproval = !data.allow_author_approval;
+    state.settings.preventAuthorApproval = !data.allow_author_approval;
     state.isLoading = false;
   },
   [types.RECEIVE_SETTINGS_ERROR](state) {
     state.isLoading = false;
   },
-  [types.UPDATE_PREVENT_AUTHOR_APPROVAL](state, value) {
-    state.preventAuthorApproval = value;
+  [types.REQUEST_UPDATE_SETTINGS](state) {
+    state.isLoading = true;
+  },
+  [types.UPDATE_SETTINGS_SUCCESS](state, data) {
+    state.settings.preventAuthorApproval = !data.allow_author_approval;
+    state.isLoading = false;
+  },
+  [types.UPDATE_SETTINGS_ERROR](state) {
+    state.isLoading = false;
   },
 };

ee/app/assets/javascripts/approvals/stores/modules/group_settings/state.js

 export default () => ({
-  preventAuthorApproval: true,
+  settings: {},
   isLoading: false,
 });

ee/spec/features/groups/group_settings_spec.rb

@@ -5,13 +5,16 @@ require 'spec_helper'
 RSpec.describe 'Edit group settings' do
   include Select2Helper
 
-  let(:user) { create(:user) }
-  let(:developer) { create(:user) }
-  let(:group) { create(:group, path: 'foo') }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:developer) { create(:user) }
+  let_it_be(:group) { create(:group, path: 'foo') }
 
-  before do
+  before_all do
     group.add_owner(user)
     group.add_developer(developer)
+  end
+
+  before do
     sign_in(user)
   end
 
@@ -275,16 +278,38 @@ RSpec.describe 'Edit group settings' do
   end
 
   describe 'merge request approval settings', :js do
+    let_it_be(:approval_settings) do
+      create(:group_merge_request_approval_setting, group: group, allow_author_approval: false)
+    end
+
     context 'when feature flag is enabled and group is licensed' do
       before do
         stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
         stub_licensed_features(group_merge_request_approval_settings: true)
       end
 
-      it 'is visible' do
+      it 'allows to save settings' do
         visit edit_group_path(group)
+        wait_for_all_requests
 
         expect(page).to have_content('Merge request approvals')
+
+        within('[data-testid="merge-request-approval-settings"]') do
+          click_button 'Expand'
+          expect(find('[data-testid="prevent-author-approval"]')).to be_checked
+
+          find('[data-testid="prevent-author-approval"]').set(false)
+          click_button 'Save changes'
+          wait_for_all_requests
+        end
+
+        visit edit_group_path(group)
+        wait_for_all_requests
+
+        within('[data-testid="merge-request-approval-settings"]') do
+          click_button 'Expand'
+          expect(find('[data-testid="prevent-author-approval"]')).not_to be_checked
+        end
       end
     end
 

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

-import { GlButton } from '@gitlab/ui';
+import { GlButton, GlForm } from '@gitlab/ui';
 import { createLocalVue, shallowMount } from '@vue/test-utils';
 import Vuex from 'vuex';
 
@@ -24,6 +24,7 @@ describe('ApprovalSettings', () => {
     });
   };
 
+  const findForm = () => wrapper.findComponent(GlForm);
   const findPreventAuthorApproval = () => wrapper.find('[data-testid="prevent-author-approval"]');
   const findSaveButton = () => wrapper.findComponent(GlButton);
 
@@ -31,6 +32,7 @@ describe('ApprovalSettings', () => {
     store = createStoreOptions(groupSettingsModule());
 
     jest.spyOn(store.modules.approvals.actions, 'fetchSettings').mockImplementation();
+    jest.spyOn(store.modules.approvals.actions, 'updateSettings').mockImplementation();
     ({ actions } = store.modules.approvals);
   });
 
@@ -52,8 +54,7 @@ describe('ApprovalSettings', () => {
       const input = findPreventAuthorApproval();
       await input.vm.$emit('input', false);
 
-      expect(input.vm.$attrs.checked).toBe(false);
-      expect(store.modules.approvals.state.preventAuthorApproval).toBe(false);
+      expect(store.modules.approvals.state.settings.preventAuthorApproval).toBe(false);
     });
   });
 
@@ -74,4 +75,14 @@ describe('ApprovalSettings', () => {
       expect(findSaveButton().props('disabled')).toBe(true);
     });
   });
+
+  describe('form submission', () => {
+    it('update settings via API', async () => {
+      createWrapper();
+
+      await findForm().vm.$emit('submit', { preventDefault: () => {} });
+
+      expect(actions.updateSettings).toHaveBeenCalledWith(expect.any(Object), approvalSettingsPath);
+    });
+  });
 });

ee/spec/frontend/approvals/stores/modules/group_settings/actions_spec.js

@@ -69,17 +69,55 @@ describe('EE approvals group settings module actions', () => {
     });
   });
 
-  describe('updatePreventAuthorApproval', () => {
-    it('updates payload', () => {
-      const value = false;
+  describe('updateSettings', () => {
+    beforeEach(() => {
+      state = {
+        settings: {
+          preventAuthorApproval: false,
+        },
+      };
+    });
+
+    describe('on success', () => {
+      it('dispatches the request and updates payload', () => {
+        const data = { allow_author_approval: true };
+        mock.onPut(approvalSettingsPath).replyOnce(httpStatus.OK, data);
+
+        return testAction(
+          actions.updateSettings,
+          approvalSettingsPath,
+          state,
+          [
+            { type: types.REQUEST_UPDATE_SETTINGS },
+            { type: types.UPDATE_SETTINGS_SUCCESS, payload: data },
+          ],
+          [],
+        );
+      });
+    });
+
+    describe('on error', () => {
+      it('dispatches the request, updates payload and sets error message', () => {
+        const data = { message: 'Internal Server Error' };
+        mock.onPut(approvalSettingsPath).replyOnce(httpStatus.INTERNAL_SERVER_ERROR, data);
 
-      return testAction(
-        actions.updatePreventAuthorApproval,
-        value,
-        state,
-        [{ type: types.UPDATE_PREVENT_AUTHOR_APPROVAL, payload: value }],
-        [],
-      );
+        return testAction(
+          actions.updateSettings,
+          approvalSettingsPath,
+          state,
+          [
+            { type: types.REQUEST_UPDATE_SETTINGS },
+            { type: types.UPDATE_SETTINGS_ERROR, payload: data.message },
+          ],
+          [],
+        ).then(() => {
+          expect(createFlash).toHaveBeenCalledWith({
+            message: 'There was an error updating merge request approval settings.',
+            captureError: true,
+            error: 'Internal Server Error',
+          });
+        });
+      });
     });
   });
 });

ee/spec/frontend/approvals/stores/modules/group_settings/mutations_spec.js

@@ -20,7 +20,7 @@ describe('Group settings store mutations', () => {
     it('updates settings', () => {
       mutations.RECEIVE_SETTINGS_SUCCESS(state, { allow_author_approval: true });
 
-      expect(state.preventAuthorApproval).toBe(false);
+      expect(state.settings.preventAuthorApproval).toBe(false);
       expect(state.isLoading).toBe(false);
     });
   });
@@ -33,11 +33,28 @@ describe('Group settings store mutations', () => {
     });
   });
 
-  describe('UPDATE_PREVENT_AUTHOR_APPROVAL', () => {
-    it('updates setting', () => {
-      mutations.UPDATE_PREVENT_AUTHOR_APPROVAL(state, false);
+  describe('REQUEST_UPDATE_SETTINGS', () => {
+    it('sets loading state', () => {
+      mutations.REQUEST_UPDATE_SETTINGS(state);
+
+      expect(state.isLoading).toBe(true);
+    });
+  });
+
+  describe('UPDATE_SETTINGS_SUCCESS', () => {
+    it('updates settings', () => {
+      mutations.UPDATE_SETTINGS_SUCCESS(state, { allow_author_approval: true });
 
-      expect(state.preventAuthorApproval).toBe(false);
+      expect(state.settings.preventAuthorApproval).toBe(false);
+      expect(state.isLoading).toBe(false);
+    });
+  });
+
+  describe('UPDATE_SETTINGS_ERROR', () => {
+    it('sets loading state', () => {
+      mutations.UPDATE_SETTINGS_ERROR(state);
+
+      expect(state.isLoading).toBe(false);
     });
   });
 });

locale/gitlab.pot

@@ -29695,6 +29695,9 @@ msgstr ""
 msgid "There was an error trying to validate your query"
 msgstr ""
 
+msgid "There was an error updating merge request approval settings."
+msgstr ""
+
 msgid "There was an error updating the Geo Settings"
 msgstr ""