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 6dfc7c2b520835b72e32cef6398028f2ce5b1850..a679154c0fdb57e7c8bad6d74c2e459749fae655 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/changelogs/unreleased/213363-better-node-errors.yml b/ee/changelogs/unreleased/213363-better-node-errors.yml new file mode 100644 index 0000000000000000000000000000000000000000..b316c6c58910579962c5aca433870910dd334061 --- /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 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 db9d3311cc087f21705a56357b557f170018ed18..082fd4ac33b782b876dfb7ed4633a2b8d34a4d36 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 4549c7fc4cf97d1b75688fa3ab0c665b8d537b1e..341b2b9fbe513f233acdc0ce77e6ae4a555e8318 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 981948cdd7295f67c66b96068e8034a1996294cb..b13b762c2c3d2bb198be44a35ffd63919c1d79ef 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 7e28e80aebefd223f1bb6c80c458fd78c45274e5..cc89d307e764eacf429e6bf802f4136a1677cd8c 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."