Merge branch...

Merge branch '335982-add-loading-state-and-form-hiding-on-error-to-group-level-merge-request-settings' into 'master'

Add loading icon and form hiding on error to group-level merge request settings

See merge request gitlab-org/gitlab!66154

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

 <script>
-import { GlButton, GlForm, GlFormGroup } from '@gitlab/ui';
+import { GlAlert, GlButton, GlForm, GlFormGroup, GlLoadingIcon } from '@gitlab/ui';
+import { isEmpty } from 'lodash';
 import { mapActions, mapState } from 'vuex';
 import { mapComputed } from '~/vuex_shared/bindings';
 import { APPROVAL_SETTINGS_I18N } from '../constants';
@@ -8,9 +9,11 @@ import ApprovalSettingsCheckbox from './approval_settings_checkbox.vue';
 export default {
   components: {
     ApprovalSettingsCheckbox,
+    GlAlert,
     GlButton,
     GlForm,
     GlFormGroup,
+    GlLoadingIcon,
   },
   props: {
     approvalSettingsPath: {
@@ -18,14 +21,12 @@ export default {
       required: true,
     },
   },
-  data() {
-    return {
-      hasFormLoaded: false,
-    };
-  },
   computed: {
     ...mapState({
       isLoading: (state) => state.approvals.isLoading,
+      isUpdated: (state) => state.approvals.isUpdated,
+      settings: (state) => state.approvals.settings,
+      errorMessage: (state) => state.approvals.errorMessage,
     }),
     ...mapComputed(
       [
@@ -38,15 +39,25 @@ export default {
       undefined,
       (state) => state.approvals.settings,
     ),
+    hasSettings() {
+      return !isEmpty(this.settings);
+    },
+    isLoaded() {
+      return this.hasSettings || this.errorMessage;
+    },
   },
-  async created() {
-    await this.fetchSettings(this.approvalSettingsPath);
-    this.hasFormLoaded = true;
+  created() {
+    this.fetchSettings(this.approvalSettingsPath);
   },
   methods: {
-    ...mapActions(['fetchSettings', 'updateSettings']),
-    onSubmit() {
-      this.updateSettings(this.approvalSettingsPath);
+    ...mapActions([
+      'fetchSettings',
+      'updateSettings',
+      'dismissErrorMessage',
+      'dismissSuccessMessage',
+    ]),
+    async onSubmit() {
+      await this.updateSettings(this.approvalSettingsPath);
     },
   },
   links: {
@@ -62,41 +73,66 @@ export default {
 </script>
 
 <template>
-  <gl-form v-if="hasFormLoaded" @submit.prevent="onSubmit">
-    <gl-form-group>
-      <approval-settings-checkbox
-        v-model="preventAuthorApproval"
-        :label="$options.i18n.authorApprovalLabel"
-        :anchor="$options.links.preventAuthorApprovalDocsAnchor"
-        data-testid="prevent-author-approval"
-      />
-      <approval-settings-checkbox
-        v-model="preventMrApprovalRuleEdit"
-        :label="$options.i18n.preventMrApprovalRuleEditLabel"
-        :anchor="$options.links.preventMrApprovalRuleEditDocsAnchor"
-        data-testid="prevent-mr-approval-rule-edit"
-      />
-      <approval-settings-checkbox
-        v-model="requireUserPassword"
-        :label="$options.i18n.requireUserPasswordLabel"
-        :anchor="$options.links.requireUserPasswordDocsAnchor"
-        data-testid="require-user-password"
-      />
-      <approval-settings-checkbox
-        v-model="removeApprovalsOnPush"
-        :label="$options.i18n.removeApprovalsOnPushLabel"
-        :anchor="$options.links.removeApprovalsOnPushDocsAnchor"
-        data-testid="remove-approvals-on-push"
-      />
-      <approval-settings-checkbox
-        v-model="preventCommittersApproval"
-        :label="$options.i18n.preventCommittersApprovalLabel"
-        :anchor="$options.links.preventCommittersApprovalAnchor"
-        data-testid="prevent-committers-approval"
-      />
-    </gl-form-group>
-    <gl-button type="submit" variant="success" category="primary" :disabled="isLoading">
-      {{ $options.i18n.saveChanges }}
-    </gl-button>
-  </gl-form>
+  <div>
+    <gl-loading-icon v-if="!isLoaded" size="lg" />
+    <gl-alert
+      v-if="errorMessage"
+      variant="danger"
+      :dismissible="hasSettings"
+      :contained="true"
+      :class="{ 'gl-mb-6': hasSettings }"
+      data-testid="error-alert"
+      @dismiss="dismissErrorMessage"
+    >
+      {{ errorMessage }}
+    </gl-alert>
+    <gl-alert
+      v-if="isUpdated"
+      variant="success"
+      :dismissible="true"
+      :contained="true"
+      class="gl-mb-6"
+      data-testid="success-alert"
+      @dismiss="dismissSuccessMessage"
+    >
+      {{ $options.i18n.savingSuccessMessage }}
+    </gl-alert>
+    <gl-form v-if="hasSettings" @submit.prevent="onSubmit">
+      <gl-form-group>
+        <approval-settings-checkbox
+          v-model="preventAuthorApproval"
+          :label="$options.i18n.authorApprovalLabel"
+          :anchor="$options.links.preventAuthorApprovalDocsAnchor"
+          data-testid="prevent-author-approval"
+        />
+        <approval-settings-checkbox
+          v-model="preventMrApprovalRuleEdit"
+          :label="$options.i18n.preventMrApprovalRuleEditLabel"
+          :anchor="$options.links.preventMrApprovalRuleEditDocsAnchor"
+          data-testid="prevent-mr-approval-rule-edit"
+        />
+        <approval-settings-checkbox
+          v-model="requireUserPassword"
+          :label="$options.i18n.requireUserPasswordLabel"
+          :anchor="$options.links.requireUserPasswordDocsAnchor"
+          data-testid="require-user-password"
+        />
+        <approval-settings-checkbox
+          v-model="removeApprovalsOnPush"
+          :label="$options.i18n.removeApprovalsOnPushLabel"
+          :anchor="$options.links.removeApprovalsOnPushDocsAnchor"
+          data-testid="remove-approvals-on-push"
+        />
+        <approval-settings-checkbox
+          v-model="preventCommittersApproval"
+          :label="$options.i18n.preventCommittersApprovalLabel"
+          :anchor="$options.links.preventCommittersApprovalAnchor"
+          data-testid="prevent-committers-approval"
+        />
+      </gl-form-group>
+      <gl-button type="submit" variant="confirm" category="primary" :loading="isLoading">
+        {{ $options.i18n.saveChanges }}
+      </gl-button>
+    </gl-form>
+  </div>
 </template>

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

@@ -50,14 +50,23 @@ export const APPROVAL_RULE_CONFIGS = {
 export const APPROVALS_HELP_PATH = 'user/project/merge_requests/merge_request_approvals';
 
 export const APPROVAL_SETTINGS_I18N = {
-  authorApprovalLabel: __('Prevent MR approvals by the author.'),
-  preventMrApprovalRuleEditLabel: __('Prevent users from modifying MR approval rules.'),
-  preventCommittersApprovalLabel: __(
-    'Prevent approval of merge requests by merge request committers.',
+  authorApprovalLabel: s__('ApprovalSettings|Prevent MR approvals by the author.'),
+  preventMrApprovalRuleEditLabel: s__(
+    'ApprovalSettings|Prevent users from modifying MR approval rules.',
   ),
-  requireUserPasswordLabel: __('Require user password for approvals.'),
-  removeApprovalsOnPushLabel: __(
-    'Remove all approvals in a merge request when new commits are pushed to its source branch.',
+  preventCommittersApprovalLabel: s__(
+    'ApprovalSettings|Prevent approval of merge requests by merge request committers.',
+  ),
+  requireUserPasswordLabel: s__('ApprovalSettings|Require user password for approvals.'),
+  removeApprovalsOnPushLabel: s__(
+    'ApprovalSettings|Remove all approvals in a merge request when new commits are pushed to its source branch.',
   ),
   saveChanges: __('Save changes'),
+  loadingErrorMessage: s__(
+    'ApprovalSettings|There was an error loading merge request approval settings.',
+  ),
+  savingErrorMessage: s__(
+    'ApprovalSettings|There was an error updating merge request approval settings.',
+  ),
+  savingSuccessMessage: s__('ApprovalSettings|Merge request approval settings have been updated.'),
 };

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

-import createFlash from '~/flash';
+import * as Sentry from '@sentry/browser';
 import axios from '~/lib/utils/axios_utils';
-import { __ } from '~/locale';
 import * as types from './mutation_types';
 
 const mapStateToPayload = (state) => ({
@@ -19,15 +18,11 @@ export const fetchSettings = ({ commit }, endpoint) => {
     .then(({ data }) => {
       commit(types.RECEIVE_SETTINGS_SUCCESS, data);
     })
-    .catch(({ response }) => {
-      const error = response?.data?.message;
+    .catch((e) => {
+      const error = e?.response?.data?.message || e;
 
-      commit(types.RECEIVE_SETTINGS_ERROR, error);
-      createFlash({
-        message: __('There was an error loading merge request approval settings.'),
-        captureError: true,
-        error,
-      });
+      Sentry.captureException(error);
+      commit(types.RECEIVE_SETTINGS_ERROR);
     });
 };
 
@@ -38,23 +33,23 @@ export const updateSettings = ({ commit, state }, endpoint) => {
     .put(endpoint, { ...mapStateToPayload(state) })
     .then(({ data }) => {
       commit(types.UPDATE_SETTINGS_SUCCESS, data);
-      createFlash({
-        message: __('Merge request approval settings have been updated.'),
-        type: 'notice',
-      });
     })
-    .catch(({ response }) => {
-      const error = response?.data?.message;
+    .catch((e) => {
+      const error = e?.response?.data?.message || e;
 
-      commit(types.UPDATE_SETTINGS_ERROR, error);
-      createFlash({
-        message: __('There was an error updating merge request approval settings.'),
-        captureError: true,
-        error,
-      });
+      Sentry.captureException(error);
+      commit(types.UPDATE_SETTINGS_ERROR);
     });
 };
 
+export const dismissSuccessMessage = ({ commit }) => {
+  commit(types.DISMISS_SUCCESS_MESSAGE);
+};
+
+export const dismissErrorMessage = ({ commit }) => {
+  commit(types.DISMISS_ERROR_MESSAGE);
+};
+
 export const setPreventAuthorApproval = ({ commit }, { preventAuthorApproval }) => {
   commit(types.SET_PREVENT_AUTHOR_APPROVAL, preventAuthorApproval);
 };

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

@@ -6,6 +6,8 @@ export const RECEIVE_SETTINGS_ERROR = 'RECEIVE_SETTINGS_ERROR';
 export const REQUEST_UPDATE_SETTINGS = 'REQUEST_UPDATE_SETTINGS';
 export const UPDATE_SETTINGS_SUCCESS = 'UPDATE_SETTINGS_SUCCESS';
 export const UPDATE_SETTINGS_ERROR = 'UPDATE_SETTINGS_ERROR';
+export const DISMISS_SUCCESS_MESSAGE = 'DISMISS_SUCCESS_MESSAGE';
+export const DISMISS_ERROR_MESSAGE = 'DISMISS_ERROR_MESSAGE';
 
 export const SET_PREVENT_AUTHOR_APPROVAL = 'SET_PREVENT_AUTHOR_APPROVAL';
 export const SET_PREVENT_COMMITTERS_APPROVAL = 'SET_PREVENT_COMMITTERS_APPROVAL';

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

+import { APPROVAL_SETTINGS_I18N } from '../../../constants';
 import * as types from './mutation_types';
 
 const mapDataToState = (data) => ({
@@ -11,6 +12,7 @@ const mapDataToState = (data) => ({
 export default {
   [types.REQUEST_SETTINGS](state) {
     state.isLoading = true;
+    state.errorMessage = '';
   },
   [types.RECEIVE_SETTINGS_SUCCESS](state, data) {
     state.settings = { ...mapDataToState(data) };
@@ -18,16 +20,27 @@ export default {
   },
   [types.RECEIVE_SETTINGS_ERROR](state) {
     state.isLoading = false;
+    state.errorMessage = APPROVAL_SETTINGS_I18N.loadingErrorMessage;
   },
   [types.REQUEST_UPDATE_SETTINGS](state) {
     state.isLoading = true;
+    state.isUpdated = false;
+    state.errorMessage = '';
   },
   [types.UPDATE_SETTINGS_SUCCESS](state, data) {
     state.settings = { ...mapDataToState(data) };
     state.isLoading = false;
+    state.isUpdated = true;
   },
   [types.UPDATE_SETTINGS_ERROR](state) {
     state.isLoading = false;
+    state.errorMessage = APPROVAL_SETTINGS_I18N.savingErrorMessage;
+  },
+  [types.DISMISS_SUCCESS_MESSAGE](state) {
+    state.isUpdated = false;
+  },
+  [types.DISMISS_ERROR_MESSAGE](state) {
+    state.errorMessage = '';
   },
   [types.SET_PREVENT_AUTHOR_APPROVAL](state, preventAuthorApproval) {
     state.settings.preventAuthorApproval = preventAuthorApproval;

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

 export default () => ({
   settings: {},
   isLoading: false,
+  isUpdated: false,
+  errorMessage: '',
 });

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

+import * as Sentry from '@sentry/browser';
 import MockAdapter from 'axios-mock-adapter';
 import * as actions from 'ee/approvals/stores/modules/group_settings/actions';
 import * as types from 'ee/approvals/stores/modules/group_settings/mutation_types';
 import getInitialState from 'ee/approvals/stores/modules/group_settings/state';
 import testAction from 'helpers/vuex_action_helper';
-import createFlash from '~/flash';
 import axios from '~/lib/utils/axios_utils';
 import httpStatus from '~/lib/utils/http_status';
 
-jest.mock('~/flash');
-
 describe('EE approvals group settings module actions', () => {
   let state;
   let mock;
@@ -18,10 +16,10 @@ describe('EE approvals group settings module actions', () => {
   beforeEach(() => {
     state = getInitialState();
     mock = new MockAdapter(axios);
+    jest.spyOn(Sentry, 'captureException');
   });
 
   afterEach(() => {
-    createFlash.mockClear();
     mock.restore();
   });
 
@@ -53,17 +51,10 @@ describe('EE approvals group settings module actions', () => {
           actions.fetchSettings,
           approvalSettingsPath,
           state,
-          [
-            { type: types.REQUEST_SETTINGS },
-            { type: types.RECEIVE_SETTINGS_ERROR, payload: data.message },
-          ],
+          [{ type: types.REQUEST_SETTINGS }, { type: types.RECEIVE_SETTINGS_ERROR }],
           [],
         ).then(() => {
-          expect(createFlash).toHaveBeenCalledWith({
-            message: 'There was an error loading merge request approval settings.',
-            captureError: true,
-            error: 'Internal Server Error',
-          });
+          expect(Sentry.captureException.mock.calls[0][0]).toBe(data.message);
         });
       });
     });
@@ -102,12 +93,7 @@ describe('EE approvals group settings module actions', () => {
             { type: types.UPDATE_SETTINGS_SUCCESS, payload: data },
           ],
           [],
-        ).then(() => {
-          expect(createFlash).toHaveBeenCalledWith({
-            message: 'Merge request approval settings have been updated.',
-            type: 'notice',
-          });
-        });
+        );
       });
     });
 
@@ -120,22 +106,39 @@ describe('EE approvals group settings module actions', () => {
           actions.updateSettings,
           approvalSettingsPath,
           state,
-          [
-            { type: types.REQUEST_UPDATE_SETTINGS },
-            { type: types.UPDATE_SETTINGS_ERROR, payload: data.message },
-          ],
+          [{ type: types.REQUEST_UPDATE_SETTINGS }, { type: types.UPDATE_SETTINGS_ERROR }],
           [],
         ).then(() => {
-          expect(createFlash).toHaveBeenCalledWith({
-            message: 'There was an error updating merge request approval settings.',
-            captureError: true,
-            error: 'Internal Server Error',
-          });
+          expect(Sentry.captureException.mock.calls[0][0]).toBe(data.message);
         });
       });
     });
   });
 
+  describe('dismissSuccessMessage', () => {
+    it('commits DISMISS_SUCCESS_MESSAGE', () => {
+      return testAction(
+        actions.dismissSuccessMessage,
+        {},
+        state,
+        [{ type: types.DISMISS_SUCCESS_MESSAGE }],
+        [],
+      );
+    });
+  });
+
+  describe('dismissErrorMessage', () => {
+    it('commits DISMISS_ERROR_MESSAGE', () => {
+      return testAction(
+        actions.dismissErrorMessage,
+        {},
+        state,
+        [{ type: types.DISMISS_ERROR_MESSAGE }],
+        [],
+      );
+    });
+  });
+
   describe.each`
     action                            | type                                       | prop
     ${'setPreventAuthorApproval'}     | ${types.SET_PREVENT_AUTHOR_APPROVAL}       | ${'preventAuthorApproval'}

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

+import { APPROVAL_SETTINGS_I18N } from 'ee/approvals/constants';
 import mutations from 'ee/approvals/stores/modules/group_settings/mutations';
 import getInitialState from 'ee/approvals/stores/modules/group_settings/state';
 
@@ -13,6 +14,7 @@ describe('Group settings store mutations', () => {
       mutations.REQUEST_SETTINGS(state);
 
       expect(state.isLoading).toBe(true);
+      expect(state.errorMessage).toBe('');
     });
   });
 
@@ -40,6 +42,7 @@ describe('Group settings store mutations', () => {
       mutations.RECEIVE_SETTINGS_ERROR(state);
 
       expect(state.isLoading).toBe(false);
+      expect(state.errorMessage).toBe(APPROVAL_SETTINGS_I18N.loadingErrorMessage);
     });
   });
 
@@ -48,6 +51,8 @@ describe('Group settings store mutations', () => {
       mutations.REQUEST_UPDATE_SETTINGS(state);
 
       expect(state.isLoading).toBe(true);
+      expect(state.isUpdated).toBe(false);
+      expect(state.errorMessage).toBe('');
     });
   });
 
@@ -65,6 +70,7 @@ describe('Group settings store mutations', () => {
       expect(state.settings.requireUserPassword).toBe(true);
       expect(state.settings.removeApprovalsOnPush).toBe(false);
       expect(state.isLoading).toBe(false);
+      expect(state.isUpdated).toBe(true);
     });
   });
 
@@ -73,6 +79,23 @@ describe('Group settings store mutations', () => {
       mutations.UPDATE_SETTINGS_ERROR(state);
 
       expect(state.isLoading).toBe(false);
+      expect(state.errorMessage).toBe(APPROVAL_SETTINGS_I18N.savingErrorMessage);
+    });
+  });
+
+  describe('DISMISS_SUCCESS_MESSAGE', () => {
+    it('resets isUpdated', () => {
+      mutations.DISMISS_SUCCESS_MESSAGE(state);
+
+      expect(state.isUpdated).toBe(false);
+    });
+  });
+
+  describe('DISMISS_ERROR_MESSAGE', () => {
+    it('resets errorMessage', () => {
+      mutations.DISMISS_ERROR_MESSAGE(state);
+
+      expect(state.errorMessage).toBe('');
     });
   });
 

locale/gitlab.pot

@@ -4160,6 +4160,30 @@ msgstr ""
 msgid "ApprovalRule|Target branch"
 msgstr ""
 
+msgid "ApprovalSettings|Merge request approval settings have been updated."
+msgstr ""
+
+msgid "ApprovalSettings|Prevent MR approvals by the author."
+msgstr ""
+
+msgid "ApprovalSettings|Prevent approval of merge requests by merge request committers."
+msgstr ""
+
+msgid "ApprovalSettings|Prevent users from modifying MR approval rules."
+msgstr ""
+
+msgid "ApprovalSettings|Remove all approvals in a merge request when new commits are pushed to its source branch."
+msgstr ""
+
+msgid "ApprovalSettings|Require user password for approvals."
+msgstr ""
+
+msgid "ApprovalSettings|There was an error loading merge request approval settings."
+msgstr ""
+
+msgid "ApprovalSettings|There was an error updating merge request approval settings."
+msgstr ""
+
 msgid "ApprovalStatusTooltip|Adheres to separation of duties"
 msgstr ""
 
@@ -20489,9 +20513,6 @@ msgstr ""
 msgid "Merge request analytics"
 msgstr ""
 
-msgid "Merge request approval settings have been updated."
-msgstr ""
-
 msgid "Merge request approvals"
 msgstr ""
 
@@ -24767,9 +24788,6 @@ msgstr ""
 msgid "Prevent adding new members to project membership within this group"
 msgstr ""
 
-msgid "Prevent approval of merge requests by merge request committers."
-msgstr ""
-
 msgid "Prevent environment from auto-stopping"
 msgstr ""
 
@@ -24785,9 +24803,6 @@ msgstr ""
 msgid "Prevent users from modifying MR approval rules in projects and merge requests."
 msgstr ""
 
-msgid "Prevent users from modifying MR approval rules."
-msgstr ""
-
 msgid "Prevent users from performing write operations on GitLab while performing maintenance."
 msgstr ""
 
@@ -27126,9 +27141,6 @@ msgstr ""
 msgid "Remove access"
 msgstr ""
 
-msgid "Remove all approvals in a merge request when new commits are pushed to its source branch."
-msgstr ""
-
 msgid "Remove all or specific assignee(s)"
 msgstr ""
 
@@ -33126,9 +33138,6 @@ msgstr ""
 msgid "There was an error importing the Jira project."
 msgstr ""
 
-msgid "There was an error loading merge request approval settings."
-msgstr ""
-
 msgid "There was an error loading related feature flags"
 msgstr ""
 
@@ -33168,9 +33177,6 @@ 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 ""