From 34ac2c8478b8ba280dea31fde35d3330a31de3d9 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 17 Aug 2020 23:38:19 -0600 Subject: [PATCH 1/6] Remove action error handling so errors can be handled inline --- .../stores/modules/project_settings/actions.js | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) 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 803fceddbf7fe6..e916ce5d12a376 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) => { -- GitLab From 40bcda62c82bce75ee10e82cab76e998e96d326c Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 18 Aug 2020 04:41:15 -0600 Subject: [PATCH 2/6] Add server-sent name-field validation errors --- .../approvals/components/rule_form.vue | 37 +++++++++++++++---- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/ee/app/assets/javascripts/approvals/components/rule_form.vue b/ee/app/assets/javascripts/approvals/components/rule_form.vue index 339f08c08faa80..39c2a97994e131 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. -- GitLab From 8087af71199f1074dc5e79e5828f54c08d0e7285 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 18 Aug 2020 06:34:52 -0600 Subject: [PATCH 3/6] Add i18n text for the duplicate approval name warning --- locale/gitlab.pot | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 43f3e471da82dd..1e2fcd55c2e020 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 "" -- GitLab From 3f321bd96bebf63d8191805d465b03c97f150600 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 13 Aug 2020 00:26:48 -0600 Subject: [PATCH 4/6] Add changelog for the error move bugfix --- .../unreleased/defect-add-approval-validation-errors.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ee/changelogs/unreleased/defect-add-approval-validation-errors.yml 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 00000000000000..7df6b6fe79982d --- /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 -- GitLab From f3ae8d535c76378aa38b9b444f38cc50f06ac766 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 17 Aug 2020 23:37:50 -0600 Subject: [PATCH 5/6] Remove actions tests for things they no longer do Removes all the tests for any createFlash actions that happen as a result of post or put actions. --- .../modules/project_settings/actions_spec.js | 34 ------------------- 1 file changed, 34 deletions(-) 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 0d771d538a073c..26bb1e330b0ed7 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', () => { -- GitLab From 1cc1efd49bdbf66972766824fe70b95503dca58d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 24 Aug 2020 06:02:12 -0600 Subject: [PATCH 6/6] Test the UI updates when the name is already taken --- .../approvals/components/rule_form_spec.js | 76 ++++++++++++++++--- 1 file changed, 66 insertions(+), 10 deletions(-) diff --git a/ee/spec/frontend/approvals/components/rule_form_spec.js b/ee/spec/frontend/approvals/components/rule_form_spec.js index 2fde554ee88745..dff7b66ad488e6 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.', + }); + }); }); }); -- GitLab