Issue 323331 - createFlash called twice in search fetchProjects

Changelog: changed

app/assets/javascripts/api.js

@@ -234,7 +234,7 @@ const Api = {
 
     return axios
       .get(url, {
-        params: Object.assign(defaults, options),
+        params: { ...defaults, ...options },
       })
       .then(({ data, headers }) => {
         callback(data);
@@ -445,7 +445,7 @@ const Api = {
   },
 
   // Return group projects list. Filtered by query
-  groupProjects(groupId, query, options, callback) {
+  groupProjects(groupId, query, options, callback = () => {}, useCustomErrorHandler = false) {
     const url = Api.buildUrl(Api.groupProjectsPath).replace(':id', groupId);
     const defaults = {
       search: query,
@@ -455,14 +455,21 @@ const Api = {
       .get(url, {
         params: { ...defaults, ...options },
       })
-      .then(({ data }) => (callback ? callback(data) : data))
-      .catch(() => {
+      .then(({ data, headers }) => {
+        callback(data);
+
+        return { data, headers };
+      })
+      .catch((error) => {
+        if (useCustomErrorHandler) {
+          throw error;
+        }
+
         createFlash({
           message: __('Something went wrong while fetching projects'),
         });
-        if (callback) {
-          callback();
-        }
+
+        callback();
       });
   },
 

app/assets/javascripts/search/store/actions.js

@@ -18,31 +18,35 @@ export const fetchGroups = ({ commit }, search) => {
     });
 };
 
-export const fetchProjects = ({ commit, state }, search) => {
+export const fetchProjects = ({ commit, state }, search, emptyCallback = () => {}) => {
   commit(types.REQUEST_PROJECTS);
   const groupId = state.query?.group_id;
-  const callback = (data) => {
-    if (data) {
-      commit(types.RECEIVE_PROJECTS_SUCCESS, data);
-    } else {
-      createFlash({ message: __('There was an error fetching projects') });
-      commit(types.RECEIVE_PROJECTS_ERROR);
-    }
+
+  const handleCatch = () => {
+    createFlash({ message: __('There was an error fetching projects') });
+    commit(types.RECEIVE_PROJECTS_ERROR);
+  };
+  const handleSuccess = ({ data }) => {
+    commit(types.RECEIVE_PROJECTS_SUCCESS, data);
   };
 
   if (groupId) {
-    // TODO (https://gitlab.com/gitlab-org/gitlab/-/issues/323331): For errors `createFlash` is called twice; in `callback` and in `Api.groupProjects`
     Api.groupProjects(
       groupId,
       search,
-      { order_by: 'similarity', with_shared: false, include_subgroups: true },
-      callback,
-    );
+      {
+        order_by: 'similarity',
+        with_shared: false,
+        include_subgroups: true,
+      },
+      emptyCallback,
+      true,
+    )
+      .then(handleSuccess)
+      .catch(handleCatch);
   } else {
     // The .catch() is due to the API method not handling a rejection properly
-    Api.projects(search, { order_by: 'similarity' }, callback).catch(() => {
-      callback();
-    });
+    Api.projects(search, { order_by: 'similarity' }).then(handleSuccess).catch(handleCatch);
   }
 };
 

spec/frontend/__helpers__/vuex_action_helper.js

@@ -49,6 +49,7 @@ const noop = () => {};
  *   expectedActions: [],
  * })
  */
+
 export default (
   actionArg,
   payloadArg,

spec/frontend/api_spec.js

@@ -2,6 +2,9 @@ import MockAdapter from 'axios-mock-adapter';
 import Api, { DEFAULT_PER_PAGE } from '~/api';
 import axios from '~/lib/utils/axios_utils';
 import httpStatus from '~/lib/utils/http_status';
+import createFlash from '~/flash';
+
+jest.mock('~/flash');
 
 describe('Api', () => {
   const dummyApiVersion = 'v3000';
@@ -675,6 +678,33 @@ describe('Api', () => {
         done();
       });
     });
+
+    it('uses flesh on error by default', async () => {
+      const groupId = '123456';
+      const query = 'dummy query';
+      const expectedUrl = `${dummyUrlRoot}/api/${dummyApiVersion}/groups/${groupId}/projects.json`;
+      const flashCallback = (callCount) => {
+        expect(createFlash).toHaveBeenCalledTimes(callCount);
+        createFlash.mockClear();
+      };
+
+      mock.onGet(expectedUrl).reply(500, null);
+
+      const response = await Api.groupProjects(groupId, query, {}, () => {}).then(() => {
+        flashCallback(1);
+      });
+      expect(response).toBeUndefined();
+    });
+
+    it('NOT uses flesh on error with param useCustomErrorHandler', async () => {
+      const groupId = '123456';
+      const query = 'dummy query';
+      const expectedUrl = `${dummyUrlRoot}/api/${dummyApiVersion}/groups/${groupId}/projects.json`;
+
+      mock.onGet(expectedUrl).reply(500, null);
+      const apiCall = Api.groupProjects(groupId, query, {}, () => {}, true);
+      await expect(apiCall).rejects.toThrow();
+    });
   });
 
   describe('groupShareWithGroup', () => {

spec/frontend/search/store/actions_spec.js

@@ -56,7 +56,7 @@ describe('Global Search Store Actions', () => {
     ${actions.fetchGroups}   | ${{ method: 'onGet', code: 200, res: MOCK_GROUPS }}   | ${'success'} | ${[{ type: types.REQUEST_GROUPS }, { type: types.RECEIVE_GROUPS_SUCCESS, payload: MOCK_GROUPS }]}       | ${0}
     ${actions.fetchGroups}   | ${{ method: 'onGet', code: 500, res: null }}          | ${'error'}   | ${[{ type: types.REQUEST_GROUPS }, { type: types.RECEIVE_GROUPS_ERROR }]}                               | ${1}
     ${actions.fetchProjects} | ${{ method: 'onGet', code: 200, res: MOCK_PROJECTS }} | ${'success'} | ${[{ type: types.REQUEST_PROJECTS }, { type: types.RECEIVE_PROJECTS_SUCCESS, payload: MOCK_PROJECTS }]} | ${0}
-    ${actions.fetchProjects} | ${{ method: 'onGet', code: 500, res: null }}          | ${'error'}   | ${[{ type: types.REQUEST_PROJECTS }, { type: types.RECEIVE_PROJECTS_ERROR }]}                           | ${2}
+    ${actions.fetchProjects} | ${{ method: 'onGet', code: 500, res: null }}          | ${'error'}   | ${[{ type: types.REQUEST_PROJECTS }, { type: types.RECEIVE_PROJECTS_ERROR }]}                           | ${1}
   `(`axios calls`, ({ action, axiosMock, type, expectedMutations, flashCallCount }) => {
     describe(action.name, () => {
       describe(`on ${type}`, () => {
@@ -121,8 +121,8 @@ describe('Global Search Store Actions', () => {
 
     describe('when groupId is set', () => {
       it('calls Api.groupProjects with expected parameters', () => {
-        actions.fetchProjects({ commit: mockCommit, state });
-
+        const callbackTest = jest.fn();
+        actions.fetchProjects({ commit: mockCommit, state }, undefined, callbackTest);
         expect(Api.groupProjects).toHaveBeenCalledWith(
           state.query.group_id,
           state.query.search,
@@ -131,7 +131,8 @@ describe('Global Search Store Actions', () => {
             include_subgroups: true,
             with_shared: false,
           },
-          expect.any(Function),
+          callbackTest,
+          true,
         );
         expect(Api.projects).not.toHaveBeenCalled();
       });
@@ -144,15 +145,10 @@ describe('Global Search Store Actions', () => {
 
       it('calls Api.projects', () => {
         actions.fetchProjects({ commit: mockCommit, state });
-
         expect(Api.groupProjects).not.toHaveBeenCalled();
-        expect(Api.projects).toHaveBeenCalledWith(
-          state.query.search,
-          {
-            order_by: 'similarity',
-          },
-          expect.any(Function),
-        );
+        expect(Api.projects).toHaveBeenCalledWith(state.query.search, {
+          order_by: 'similarity',
+        });
       });
     });
   });