Merge branch '213363-better-node-errors' into 'master'

Better Geo Node Error Messages MVC

Closes #213363

See merge request gitlab-org/gitlab!29079

ee/app/assets/javascripts/geo_node_form/store/actions.js

-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);
     });
 };

ee/changelogs/unreleased/213363-better-node-errors.yml

+---
+title: Improve Geo Node save error messages
+merge_request: 29079
+author:
+type: changed

ee/spec/features/admin/geo/admin_geo_nodes_spec.rb

@@ -163,7 +163,7 @@ describe 'admin Geo Nodes', :js, :geo do
         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

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'],
+};

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();
+      });
+    });
+  });
 });

locale/gitlab.pot

@@ -8384,6 +8384,9 @@ msgstr ""
 msgid "Errors"
 msgstr ""
 
+msgid "Errors:"
+msgstr ""
+
 msgid "Estimated"
 msgstr ""
 
@@ -20774,7 +20777,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."