Merge branch '285458-group-request-approval-update-action' into 'master'

Allow users to save group MR approval settings

See merge request gitlab-org/gitlab!53975

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 @@
 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 @@
   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

@@ -29701,6 +29701,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 ""