From 3f8a717ec99147b39b70970f14fe5020661a723f Mon Sep 17 00:00:00 2001 From: Zack Cuddy Date: Tue, 7 Apr 2020 14:36:23 -0500 Subject: [PATCH 1/2] Better Geo Node Error Messages Currently the Geo Node Form just generically responds that if there was an error. The API responds with an error however. So this MR simply showcases those errors insead of the Flash message instead. --- .../geo_node_form/store/actions.js | 31 +++++++-- .../admin/geo/admin_geo_nodes_spec.rb | 2 +- ee/spec/frontend/geo_node_form/mock_data.js | 5 ++ .../geo_node_form/store/actions_spec.js | 67 ++++++++++++++----- locale/gitlab.pot | 5 +- 5 files changed, 83 insertions(+), 27 deletions(-) diff --git a/ee/app/assets/javascripts/geo_node_form/store/actions.js b/ee/app/assets/javascripts/geo_node_form/store/actions.js index 6dfc7c2b520835..a679154c0fdb57 100644 --- a/ee/app/assets/javascripts/geo_node_form/store/actions.js +++ b/ee/app/assets/javascripts/geo_node_form/store/actions.js @@ -1,11 +1,22 @@ -import Api from '~/api'; -import ApiEE from 'ee/api'; +import Api from 'ee/api'; +import { flatten } from 'lodash'; import createFlash from '~/flash'; import { visitUrl } from '~/lib/utils/url_utility'; import { convertObjectPropsToSnakeCase } from '~/lib/utils/common_utils'; import { __ } from '~/locale'; import * as types from './mutation_types'; +const getSaveErrorMessageParts = messages => { + return flatten( + Object.entries(messages || {}).map(([key, value]) => value.map(x => `${key} ${x}`)), + ); +}; + +const getSaveErrorMessage = messages => { + const parts = getSaveErrorMessageParts(messages); + return `${__('Errors:')} ${parts.join(', ')}`; +}; + export const requestSyncNamespaces = ({ commit }) => commit(types.REQUEST_SYNC_NAMESPACES); export const receiveSyncNamespacesSuccess = ({ commit }, data) => commit(types.RECEIVE_SYNC_NAMESPACES_SUCCESS, data); @@ -31,8 +42,14 @@ export const receiveSaveGeoNodeSuccess = ({ commit }) => { commit(types.RECEIVE_SAVE_GEO_NODE_COMPLETE); visitUrl('/admin/geo/nodes'); }; -export const receiveSaveGeoNodeError = ({ commit }) => { - createFlash(__(`There was an error saving this Geo Node`)); +export const receiveSaveGeoNodeError = ({ commit }, data) => { + let errorMessage = __('There was an error saving this Geo Node.'); + + if (data?.message) { + errorMessage += ` ${getSaveErrorMessage(data.message)}`; + } + + createFlash(errorMessage); commit(types.RECEIVE_SAVE_GEO_NODE_COMPLETE); }; @@ -41,9 +58,9 @@ export const saveGeoNode = ({ dispatch }, node) => { const sanitizedNode = convertObjectPropsToSnakeCase(node); const saveFunc = node.id ? 'updateGeoNode' : 'createGeoNode'; - ApiEE[saveFunc](sanitizedNode) + Api[saveFunc](sanitizedNode) .then(() => dispatch('receiveSaveGeoNodeSuccess')) - .catch(() => { - dispatch('receiveSaveGeoNodeError'); + .catch(({ response }) => { + dispatch('receiveSaveGeoNodeError', response.data); }); }; diff --git a/ee/spec/features/admin/geo/admin_geo_nodes_spec.rb b/ee/spec/features/admin/geo/admin_geo_nodes_spec.rb index db9d3311cc087f..082fd4ac33b782 100644 --- a/ee/spec/features/admin/geo/admin_geo_nodes_spec.rb +++ b/ee/spec/features/admin/geo/admin_geo_nodes_spec.rb @@ -163,7 +163,7 @@ def expect_no_fields(node_fields) wait_for_requests expect(current_path).to eq new_admin_geo_node_path - expect(page).to have_content('There was an error saving this Geo Node') + expect(page).to have_content(/There was an error saving this Geo Node.*primary node already exists/) end end end diff --git a/ee/spec/frontend/geo_node_form/mock_data.js b/ee/spec/frontend/geo_node_form/mock_data.js index 4549c7fc4cf97d..341b2b9fbe513f 100644 --- a/ee/spec/frontend/geo_node_form/mock_data.js +++ b/ee/spec/frontend/geo_node_form/mock_data.js @@ -62,3 +62,8 @@ export const MOCK_NODE = { minimumReverificationInterval: 7, syncObjectStorage: false, }; + +export const MOCK_ERROR_MESSAGE = { + name: ["can't be blank"], + url: ["can't be blank", 'must be a valid URL'], +}; diff --git a/ee/spec/frontend/geo_node_form/store/actions_spec.js b/ee/spec/frontend/geo_node_form/store/actions_spec.js index 981948cdd7295f..b13b762c2c3d2b 100644 --- a/ee/spec/frontend/geo_node_form/store/actions_spec.js +++ b/ee/spec/frontend/geo_node_form/store/actions_spec.js @@ -6,7 +6,7 @@ import { visitUrl } from '~/lib/utils/url_utility'; import * as actions from 'ee/geo_node_form/store/actions'; import * as types from 'ee/geo_node_form/store/mutation_types'; import createState from 'ee/geo_node_form/store/state'; -import { MOCK_SYNC_NAMESPACES, MOCK_NODE } from '../mock_data'; +import { MOCK_SYNC_NAMESPACES, MOCK_NODE, MOCK_ERROR_MESSAGE } from '../mock_data'; jest.mock('~/flash'); jest.mock('~/lib/utils/url_utility', () => ({ @@ -37,39 +37,70 @@ describe('GeoNodeForm Store Actions', () => { }); describe.each` - action | data | mutationName | mutationCall | callback - ${actions.requestSyncNamespaces} | ${null} | ${types.REQUEST_SYNC_NAMESPACES} | ${{ type: types.REQUEST_SYNC_NAMESPACES }} | ${noCallback} - ${actions.receiveSyncNamespacesSuccess} | ${MOCK_SYNC_NAMESPACES} | ${types.RECEIVE_SYNC_NAMESPACES_SUCCESS} | ${{ type: types.RECEIVE_SYNC_NAMESPACES_SUCCESS, payload: MOCK_SYNC_NAMESPACES }} | ${noCallback} - ${actions.receiveSyncNamespacesError} | ${null} | ${types.RECEIVE_SYNC_NAMESPACES_ERROR} | ${{ type: types.RECEIVE_SYNC_NAMESPACES_ERROR }} | ${flashCallback} - ${actions.requestSaveGeoNode} | ${null} | ${types.REQUEST_SAVE_GEO_NODE} | ${{ type: types.REQUEST_SAVE_GEO_NODE }} | ${noCallback} - ${actions.receiveSaveGeoNodeSuccess} | ${null} | ${types.RECEIVE_SAVE_GEO_NODE_COMPLETE} | ${{ type: types.RECEIVE_SAVE_GEO_NODE_COMPLETE }} | ${visitUrlCallback} - ${actions.receiveSaveGeoNodeError} | ${null} | ${types.RECEIVE_SAVE_GEO_NODE_COMPLETE} | ${{ type: types.RECEIVE_SAVE_GEO_NODE_COMPLETE }} | ${flashCallback} + action | data | mutationName | mutationCall | callback + ${actions.requestSyncNamespaces} | ${null} | ${types.REQUEST_SYNC_NAMESPACES} | ${{ type: types.REQUEST_SYNC_NAMESPACES }} | ${noCallback} + ${actions.receiveSyncNamespacesSuccess} | ${MOCK_SYNC_NAMESPACES} | ${types.RECEIVE_SYNC_NAMESPACES_SUCCESS} | ${{ type: types.RECEIVE_SYNC_NAMESPACES_SUCCESS, payload: MOCK_SYNC_NAMESPACES }} | ${noCallback} + ${actions.receiveSyncNamespacesError} | ${null} | ${types.RECEIVE_SYNC_NAMESPACES_ERROR} | ${{ type: types.RECEIVE_SYNC_NAMESPACES_ERROR }} | ${flashCallback} + ${actions.requestSaveGeoNode} | ${null} | ${types.REQUEST_SAVE_GEO_NODE} | ${{ type: types.REQUEST_SAVE_GEO_NODE }} | ${noCallback} + ${actions.receiveSaveGeoNodeSuccess} | ${null} | ${types.RECEIVE_SAVE_GEO_NODE_COMPLETE} | ${{ type: types.RECEIVE_SAVE_GEO_NODE_COMPLETE }} | ${visitUrlCallback} + ${actions.receiveSaveGeoNodeError} | ${{ message: MOCK_ERROR_MESSAGE }} | ${types.RECEIVE_SAVE_GEO_NODE_COMPLETE} | ${{ type: types.RECEIVE_SAVE_GEO_NODE_COMPLETE }} | ${flashCallback} `(`non-axios calls`, ({ action, data, mutationName, mutationCall, callback }) => { describe(action.name, () => { it(`should commit mutation ${mutationName}`, () => { - testAction(action, data, state, [mutationCall], [], callback); + return testAction(action, data, state, [mutationCall], []).then(() => callback()); }); }); }); describe.each` - action | axiosMock | data | type | actionCalls - ${actions.fetchSyncNamespaces} | ${{ method: 'onGet', code: 200, res: MOCK_SYNC_NAMESPACES }} | ${null} | ${'success'} | ${[{ type: 'requestSyncNamespaces' }, { type: 'receiveSyncNamespacesSuccess', payload: MOCK_SYNC_NAMESPACES }]} - ${actions.fetchSyncNamespaces} | ${{ method: 'onGet', code: 500, res: null }} | ${null} | ${'error'} | ${[{ type: 'requestSyncNamespaces' }, { type: 'receiveSyncNamespacesError' }]} - ${actions.saveGeoNode} | ${{ method: 'onPost', code: 200, res: { ...MOCK_NODE, id: null } }} | ${{ ...MOCK_NODE, id: null }} | ${'success'} | ${[{ type: 'requestSaveGeoNode' }, { type: 'receiveSaveGeoNodeSuccess' }]} - ${actions.saveGeoNode} | ${{ method: 'onPost', code: 500, res: null }} | ${{ ...MOCK_NODE, id: null }} | ${'error'} | ${[{ type: 'requestSaveGeoNode' }, { type: 'receiveSaveGeoNodeError' }]} - ${actions.saveGeoNode} | ${{ method: 'onPut', code: 200, res: MOCK_NODE }} | ${MOCK_NODE} | ${'success'} | ${[{ type: 'requestSaveGeoNode' }, { type: 'receiveSaveGeoNodeSuccess' }]} - ${actions.saveGeoNode} | ${{ method: 'onPut', code: 500, res: null }} | ${MOCK_NODE} | ${'error'} | ${[{ type: 'requestSaveGeoNode' }, { type: 'receiveSaveGeoNodeError' }]} + action | axiosMock | data | type | actionCalls + ${actions.fetchSyncNamespaces} | ${{ method: 'onGet', code: 200, res: MOCK_SYNC_NAMESPACES }} | ${null} | ${'success'} | ${[{ type: 'requestSyncNamespaces' }, { type: 'receiveSyncNamespacesSuccess', payload: MOCK_SYNC_NAMESPACES }]} + ${actions.fetchSyncNamespaces} | ${{ method: 'onGet', code: 500, res: null }} | ${null} | ${'error'} | ${[{ type: 'requestSyncNamespaces' }, { type: 'receiveSyncNamespacesError' }]} + ${actions.saveGeoNode} | ${{ method: 'onPost', code: 200, res: { ...MOCK_NODE, id: null } }} | ${{ ...MOCK_NODE, id: null }} | ${'success'} | ${[{ type: 'requestSaveGeoNode' }, { type: 'receiveSaveGeoNodeSuccess' }]} + ${actions.saveGeoNode} | ${{ method: 'onPost', code: 500, res: { message: MOCK_ERROR_MESSAGE } }} | ${{ ...MOCK_NODE, id: null }} | ${'error'} | ${[{ type: 'requestSaveGeoNode' }, { type: 'receiveSaveGeoNodeError', payload: { message: MOCK_ERROR_MESSAGE } }]} + ${actions.saveGeoNode} | ${{ method: 'onPut', code: 200, res: MOCK_NODE }} | ${MOCK_NODE} | ${'success'} | ${[{ type: 'requestSaveGeoNode' }, { type: 'receiveSaveGeoNodeSuccess' }]} + ${actions.saveGeoNode} | ${{ method: 'onPut', code: 500, res: { message: MOCK_ERROR_MESSAGE } }} | ${MOCK_NODE} | ${'error'} | ${[{ type: 'requestSaveGeoNode' }, { type: 'receiveSaveGeoNodeError', payload: { message: MOCK_ERROR_MESSAGE } }]} `(`axios calls`, ({ action, axiosMock, data, type, actionCalls }) => { describe(action.name, () => { describe(`on ${type}`, () => { beforeEach(() => { mock[axiosMock.method]().replyOnce(axiosMock.code, axiosMock.res); }); - it(`should dispatch the correct request and actions`, done => { - testAction(action, data, state, [], actionCalls, done); + it(`should dispatch the correct request and actions`, () => { + return testAction(action, data, state, [], actionCalls); }); }); }); }); + + describe('receiveSaveGeoNodeError', () => { + const defaultErrorMessage = 'There was an error saving this Geo Node.'; + + it('when message passed it builds the error message correctly', () => { + return testAction( + actions.receiveSaveGeoNodeError, + { message: MOCK_ERROR_MESSAGE }, + state, + [{ type: types.RECEIVE_SAVE_GEO_NODE_COMPLETE }], + [], + ).then(() => { + const errors = "Errors: name can't be blank, url can't be blank, url must be a valid URL"; + expect(flash).toHaveBeenCalledWith(`${defaultErrorMessage} ${errors}`); + flash.mockClear(); + }); + }); + + it('when no data is passed it defaults the error message', () => { + return testAction( + actions.receiveSaveGeoNodeError, + null, + state, + [{ type: types.RECEIVE_SAVE_GEO_NODE_COMPLETE }], + [], + ).then(() => { + expect(flash).toHaveBeenCalledWith(defaultErrorMessage); + flash.mockClear(); + }); + }); + }); }); diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 7e28e80aebefd2..cc89d307e764ea 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -8384,6 +8384,9 @@ msgstr "" msgid "Errors" msgstr "" +msgid "Errors:" +msgstr "" + msgid "Estimated" msgstr "" @@ -20762,7 +20765,7 @@ msgstr "" msgid "There was an error resetting user pipeline minutes." msgstr "" -msgid "There was an error saving this Geo Node" +msgid "There was an error saving this Geo Node." msgstr "" msgid "There was an error saving your changes." -- GitLab From 13800525c1805ebdcc5444356b6350ce8163b82d Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Thu, 16 Apr 2020 13:50:04 -0500 Subject: [PATCH 2/2] Add changelog entry --- ee/changelogs/unreleased/213363-better-node-errors.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ee/changelogs/unreleased/213363-better-node-errors.yml diff --git a/ee/changelogs/unreleased/213363-better-node-errors.yml b/ee/changelogs/unreleased/213363-better-node-errors.yml new file mode 100644 index 00000000000000..b316c6c5891057 --- /dev/null +++ b/ee/changelogs/unreleased/213363-better-node-errors.yml @@ -0,0 +1,5 @@ +--- +title: Improve Geo Node save error messages +merge_request: 29079 +author: +type: changed -- GitLab