Merge branch '32306-validation-errors-do-not-appear-in-add-approvers-modal' into 'master'

Move "Add Approver" Name Validation Error into Form

See merge request gitlab-org/gitlab!39389

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

@@ -13,6 +13,10 @@ const DEFAULT_NAME_FOR_LICENSE_REPORT = 'License-Check';
 const DEFAULT_NAME_FOR_VULNERABILITY_CHECK = 'Vulnerability-Check';
 const READONLY_NAMES = [DEFAULT_NAME_FOR_LICENSE_REPORT, DEFAULT_NAME_FOR_VULNERABILITY_CHECK];
 
+function mapServerResponseToValidationErrors(messages) {
+  return Object.entries(messages).flatMap(([key, msgs]) => msgs.map(msg => `${key} ${msg}`));
+}
+
 export default {
   components: {
     ApproversList,
@@ -50,6 +54,7 @@ export default {
       showValidation: false,
       isFallback: false,
       containsHiddenGroups: false,
+      serverValidationErrors: [],
       ...this.getInitialData(),
     };
     // TODO: Remove feature flag in https://gitlab.com/gitlab-org/gitlab/-/issues/235114
@@ -98,11 +103,17 @@ export default {
       return invalidObject;
     },
     invalidName() {
-      if (!this.isMultiSubmission) {
-        return '';
+      let error = '';
+
+      if (this.isMultiSubmission) {
+        if (this.serverValidationErrors.includes('name has already been taken')) {
+          error = __('Rule name is already taken.');
+        } else if (!this.name) {
+          error = __('Please provide a name');
+        }
       }
 
-      return !this.name ? __('Please provide a name') : '';
+      return error;
     },
     invalidApprovalsRequired() {
       if (!isNumber(this.approvalsRequired)) {
@@ -204,15 +215,27 @@ export default {
      * - Multi rule?
      */
     submit() {
+      let submission;
+
+      this.serverValidationErrors = [];
+
       if (!this.validate()) {
-        return Promise.resolve();
+        submission = Promise.resolve();
       } else if (this.isFallbackSubmission) {
-        return this.submitFallback();
+        submission = this.submitFallback();
       } else if (!this.isMultiSubmission) {
-        return this.submitSingleRule();
+        submission = this.submitSingleRule();
+      } else {
+        submission = this.submitRule();
       }
 
-      return this.submitRule();
+      submission.catch(failureResponse => {
+        this.serverValidationErrors = mapServerResponseToValidationErrors(
+          failureResponse?.response?.data?.message || {},
+        );
+      });
+
+      return submission;
     },
     /**
      * Submit the rule, by either put-ing or post-ing.

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

@@ -37,17 +37,12 @@ export const postRuleSuccess = ({ dispatch }) => {
   dispatch('fetchRules');
 };
 
-export const postRuleError = () => {
-  createFlash(__('An error occurred while updating approvers'));
-};
-
 export const postRule = ({ rootState, dispatch }, rule) => {
   const { rulesPath } = rootState.settings;
 
   return axios
     .post(rulesPath, mapApprovalRuleRequest(rule))
-    .then(() => dispatch('postRuleSuccess'))
-    .catch(() => dispatch('postRuleError'));
+    .then(() => dispatch('postRuleSuccess'));
 };
 
 export const putRule = ({ rootState, dispatch }, { id, ...newRule }) => {
@@ -55,8 +50,7 @@ export const putRule = ({ rootState, dispatch }, { id, ...newRule }) => {
 
   return axios
     .put(`${rulesPath}/${id}`, mapApprovalRuleRequest(newRule))
-    .then(() => dispatch('postRuleSuccess'))
-    .catch(() => dispatch('postRuleError'));
+    .then(() => dispatch('postRuleSuccess'));
 };
 
 export const deleteRuleSuccess = ({ dispatch }) => {
@@ -82,17 +76,12 @@ export const putFallbackRuleSuccess = ({ dispatch }) => {
   dispatch('fetchRules');
 };
 
-export const putFallbackRuleError = () => {
-  createFlash(__('An error occurred while saving the approval settings'));
-};
-
 export const putFallbackRule = ({ rootState, dispatch }, fallback) => {
   const { projectPath } = rootState.settings;
 
   return axios
     .put(projectPath, mapApprovalFallbackRuleRequest(fallback))
-    .then(() => dispatch('putFallbackRuleSuccess'))
-    .catch(() => dispatch('putFallbackRuleError'));
+    .then(() => dispatch('putFallbackRuleSuccess'));
 };
 
 export const requestEditRule = ({ dispatch }, rule) => {

ee/changelogs/unreleased/defect-add-approval-validation-errors.yml

+---
+title: Move Add Approver name validation error into form
+merge_request: 39389
+author:
+type: fixed

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

@@ -28,6 +28,15 @@ const TEST_FALLBACK_RULE = {
   isFallback: true,
 };
 const TEST_LOCKED_RULE_NAME = 'LOCKED_RULE';
+const nameTakenError = {
+  response: {
+    data: {
+      message: {
+        name: ['has already been taken'],
+      },
+    },
+  },
+};
 
 const localVue = createLocalVue();
 localVue.use(Vuex);
@@ -242,7 +251,7 @@ describe('EE Approvals RuleForm', () => {
           .catch(done.fail);
       });
 
-      it('on submit with data, posts rule', () => {
+      describe('with valid data', () => {
         const users = [1, 2];
         const groups = [2, 3];
         const userRecords = users.map(id => ({ id, type: TYPE_USER }));
@@ -260,16 +269,37 @@ describe('EE Approvals RuleForm', () => {
           protectedBranchIds: branches,
         };
 
+        beforeEach(() => {
           findNameInput().setValue(expected.name);
           findApprovalsRequiredInput().setValue(expected.approvalsRequired);
           wrapper.vm.approvers = groupRecords.concat(userRecords);
           wrapper.vm.branches = expected.protectedBranchIds;
+        });
 
+        it('on submit, posts rule', () => {
           wrapper.vm.submit();
 
           expect(actions.postRule).toHaveBeenCalledWith(expect.anything(), expected, undefined);
         });
 
+        it('when submitted with a duplicate name, shows the "taken name" validation', async () => {
+          store.state.settings.prefix = 'project-settings';
+          jest.spyOn(wrapper.vm, 'postRule').mockRejectedValueOnce(nameTakenError);
+
+          wrapper.vm.submit();
+
+          await wrapper.vm.$nextTick();
+          // We have to wait for two ticks because the promise needs to resolve
+          // AND the result has to update into the UI
+          await wrapper.vm.$nextTick();
+
+          expect(findNameValidation()).toEqual({
+            isValid: false,
+            feedback: 'Rule name is already taken.',
+          });
+        });
+      });
+
       it('adds selected approvers on selection', () => {
         const orig = [{ id: 7, type: TYPE_GROUP }];
         const selected = [{ id: 2, type: TYPE_USER }];
@@ -302,7 +332,7 @@ describe('EE Approvals RuleForm', () => {
         ]);
       });
 
-      it('on submit, puts rule', () => {
+      describe('with valid data', () => {
         const userRecords = TEST_RULE.users.map(x => ({ ...x, type: TYPE_USER }));
         const groupRecords = TEST_RULE.groups.map(x => ({ ...x, type: TYPE_GROUP }));
         const users = userRecords.map(x => x.id);
@@ -318,10 +348,36 @@ describe('EE Approvals RuleForm', () => {
           protectedBranchIds: [],
         };
 
+        beforeEach(() => {
+          findNameInput().setValue(expected.name);
+          findApprovalsRequiredInput().setValue(expected.approvalsRequired);
+          wrapper.vm.approvers = groupRecords.concat(userRecords);
+          wrapper.vm.branches = expected.protectedBranchIds;
+        });
+
+        it('on submit, puts rule', () => {
           wrapper.vm.submit();
 
           expect(actions.putRule).toHaveBeenCalledWith(expect.anything(), expected, undefined);
         });
+
+        it('when submitted with a duplicate name, shows the "taken name" validation', async () => {
+          store.state.settings.prefix = 'project-settings';
+          jest.spyOn(wrapper.vm, 'putRule').mockRejectedValueOnce(nameTakenError);
+
+          wrapper.vm.submit();
+
+          await wrapper.vm.$nextTick();
+          // We have to wait for two ticks because the promise needs to resolve
+          // AND the result has to update into the UI
+          await wrapper.vm.$nextTick();
+
+          expect(findNameValidation()).toEqual({
+            isValid: false,
+            feedback: 'Rule name is already taken.',
+          });
+        });
+      });
     });
 
     describe('with init fallback rule', () => {

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

@@ -131,16 +131,6 @@ describe('EE approvals project settings module actions', () => {
     });
   });
 
-  describe('postRuleError', () => {
-    it('creates a flash', () => {
-      expect(createFlash).not.toHaveBeenCalled();
-
-      actions.postRuleError();
-
-      expect(createFlash.mock.calls[0]).toEqual([expect.stringMatching('error occurred')]);
-    });
-  });
-
   describe('postRule', () => {
     it('dispatches success on success', () => {
       mock.onPost(TEST_RULES_PATH).replyOnce(200);
@@ -161,18 +151,6 @@ describe('EE approvals project settings module actions', () => {
         },
       );
     });
-
-    it('dispatches error on error', () => {
-      mock.onPost(TEST_RULES_PATH).replyOnce(500);
-
-      return testAction(
-        actions.postRule,
-        TEST_RULE_REQUEST,
-        state,
-        [],
-        [{ type: 'postRuleError' }],
-      );
-    });
   });
 
   describe('putRule', () => {
@@ -195,18 +173,6 @@ describe('EE approvals project settings module actions', () => {
         },
       );
     });
-
-    it('dispatches error on error', () => {
-      mock.onPut(`${TEST_RULES_PATH}/${TEST_RULE_ID}`).replyOnce(500);
-
-      return testAction(
-        actions.putRule,
-        { id: TEST_RULE_ID, ...TEST_RULE_REQUEST },
-        state,
-        [],
-        [{ type: 'postRuleError' }],
-      );
-    });
   });
 
   describe('deleteRuleSuccess', () => {

locale/gitlab.pot

@@ -2834,9 +2834,6 @@ msgstr ""
 msgid "An error occurred while saving assignees"
 msgstr ""
 
-msgid "An error occurred while saving the approval settings"
-msgstr ""
-
 msgid "An error occurred while saving the template. Please check if the template exists."
 msgstr ""
 
@@ -21124,6 +21121,9 @@ msgstr ""
 msgid "Rook"
 msgstr ""
 
+msgid "Rule name is already taken."
+msgstr ""
+
 msgid "Rules that define what git pushes are accepted for a project in this group. All newly created projects in this group will use these settings."
 msgstr ""