diff --git a/ee/app/assets/javascripts/approvals/components/rule_form.vue b/ee/app/assets/javascripts/approvals/components/rule_form.vue index 339f08c08faa80f77ad7f7141d7fa89ed6ca4e79..39c2a97994e131347b4b0b1d2745e4a1b818dbb2 100644 --- a/ee/app/assets/javascripts/approvals/components/rule_form.vue +++ b/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. diff --git a/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js b/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js index 803fceddbf7fe6812b62e5222351b19f33aae9ae..e916ce5d12a3765d286d753f49f319fb91bbc827 100644 --- a/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js +++ b/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) => { diff --git a/ee/changelogs/unreleased/defect-add-approval-validation-errors.yml b/ee/changelogs/unreleased/defect-add-approval-validation-errors.yml new file mode 100644 index 0000000000000000000000000000000000000000..7df6b6fe79982de82f311a2f28a23e2053c8b028 --- /dev/null +++ b/ee/changelogs/unreleased/defect-add-approval-validation-errors.yml @@ -0,0 +1,5 @@ +--- +title: Move Add Approver name validation error into form +merge_request: 39389 +author: +type: fixed diff --git a/ee/spec/frontend/approvals/components/rule_form_spec.js b/ee/spec/frontend/approvals/components/rule_form_spec.js index 2fde554ee887453546a867cf5d097e0690812197..dff7b66ad488e6b786d1f1465fb605376dc76420 100644 --- a/ee/spec/frontend/approvals/components/rule_form_spec.js +++ b/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,14 +269,35 @@ describe('EE Approvals RuleForm', () => { protectedBranchIds: branches, }; - findNameInput().setValue(expected.name); - findApprovalsRequiredInput().setValue(expected.approvalsRequired); - wrapper.vm.approvers = groupRecords.concat(userRecords); - wrapper.vm.branches = expected.protectedBranchIds; + beforeEach(() => { + findNameInput().setValue(expected.name); + findApprovalsRequiredInput().setValue(expected.approvalsRequired); + wrapper.vm.approvers = groupRecords.concat(userRecords); + wrapper.vm.branches = expected.protectedBranchIds; + }); - wrapper.vm.submit(); + 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); - expect(actions.postRule).toHaveBeenCalledWith(expect.anything(), expected, undefined); + 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', () => { @@ -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,9 +348,35 @@ describe('EE Approvals RuleForm', () => { protectedBranchIds: [], }; - wrapper.vm.submit(); + 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); + }); - 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.', + }); + }); }); }); diff --git a/ee/spec/frontend/approvals/stores/modules/project_settings/actions_spec.js b/ee/spec/frontend/approvals/stores/modules/project_settings/actions_spec.js index 0d771d538a073cdc07caded1abafa29ab0e892c8..26bb1e330b0ed7e8a576f774576b1ca2e4e171ec 100644 --- a/ee/spec/frontend/approvals/stores/modules/project_settings/actions_spec.js +++ b/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', () => { diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 43f3e471da82dd383202ec6e627cdf3ae41e21fa..1e2fcd55c2e020615b0aa50962e8c5dd25d5ff07 100644 --- a/locale/gitlab.pot +++ b/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 "" @@ -21094,6 +21091,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 ""