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.

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/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', () => ({
@@ -43,11 +43,11 @@ describe('GeoNodeForm Store Actions', () => {
     ${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}
+    ${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());
       });
     });
   });
@@ -57,19 +57,50 @@ describe('GeoNodeForm Store Actions', () => {
     ${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: '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: null }}                        | ${MOCK_NODE}                  | ${'error'}   | ${[{ type: 'requestSaveGeoNode' }, { type: 'receiveSaveGeoNodeError' }]}
+    ${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 ""
 
@@ -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."