Apply code review suggestions

- Added specs for util methods
- Added specs for error cases
- Cleaned up the code some

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

@@ -2,6 +2,7 @@ import * as types from './mutation_types';
 import axios from '~/lib/utils/axios_utils';
 import Api from '~/api';
 import createFlash from '~/flash';
+import { __ } from '~/locale';
 import { prepareDataForApi, prepareDataForDisplay, prepareEnvironments } from './utils';
 
 export const toggleValues = ({ commit }, valueState) => {
@@ -90,9 +91,7 @@ export const requestVariables = ({ commit }) => {
 export const receiveVariablesSuccess = ({ commit }, variables) => {
   commit(types.RECEIVE_VARIABLES_SUCCESS, variables);
 };
-export const receiveVariablesError = ({ commit }, error) => {
-  commit(types.RECEIVE_VARIABLES_ERROR, error);
-};
+
 export const fetchVariables = ({ dispatch, state }) => {
   dispatch('requestVariables');
 
@@ -101,9 +100,8 @@ export const fetchVariables = ({ dispatch, state }) => {
     .then(({ data }) => {
       dispatch('receiveVariablesSuccess', prepareDataForDisplay(data.variables));
     })
-    .catch(error => {
-      createFlash(error.response.data[0]);
-      dispatch('receiveVariablesError', error);
+    .catch(() => {
+      createFlash(__('There was an error fetching the variables.'));
     });
 };
 
@@ -144,10 +142,6 @@ export const receiveEnvironmentsSuccess = ({ commit }, environments) => {
   commit(types.RECEIVE_ENVIRONMENTS_SUCCESS, environments);
 };
 
-export const receiveEnvironmentsError = ({ commit }, error) => {
-  commit(types.RECEIVE_ENVIRONMENTS_ERROR, error);
-};
-
 export const fetchEnvironments = ({ dispatch, state }) => {
   dispatch('requestEnvironments');
 
@@ -155,8 +149,7 @@ export const fetchEnvironments = ({ dispatch, state }) => {
     .then(res => {
       dispatch('receiveEnvironmentsSuccess', prepareEnvironments(res.data));
     })
-    .catch(error => {
-      createFlash(error.response.data[0]);
-      dispatch('receiveEnvironmentsError', error);
+    .catch(() => {
+      createFlash(__('There was an error fetching the environments information.'));
     });
 };

app/assets/javascripts/ci_variable_list/store/mutation_types.js

@@ -5,7 +5,6 @@ export const CLEAR_MODAL = 'CLEAR_MODAL';
 
 export const REQUEST_VARIABLES = 'REQUEST_VARIABLES';
 export const RECEIVE_VARIABLES_SUCCESS = 'RECEIVE_VARIABLES_SUCCESS';
-export const RECEIVE_VARIABLES_ERROR = 'RECEIVE_VARIABLES_ERROR';
 
 export const REQUEST_DELETE_VARIABLE = 'REQUEST_DELETE_VARIABLE';
 export const RECEIVE_DELETE_VARIABLE_SUCCESS = 'RECEIVE_DELETE_VARIABLE_SUCCESS';
@@ -21,4 +20,3 @@ export const RECEIVE_UPDATE_VARIABLE_ERROR = 'RECEIVE_UPDATE_VARIABLE_ERROR';
 
 export const REQUEST_ENVIRONMENTS = 'REQUEST_ENVIRONMENTS';
 export const RECEIVE_ENVIRONMENTS_SUCCESS = 'RECEIVE_ENVIRONMENTS_SUCCESS';
-export const RECEIVE_ENVIRONMENTS_ERROR = 'RECEIVE_ENVIRONMENTS_ERROR';

app/assets/javascripts/ci_variable_list/store/mutations.js

@@ -11,10 +11,6 @@ export default {
     state.variables = variables;
   },
 
-  [types.RECEIVE_VARIABLES_ERROR](state) {
-    state.isLoading = false;
-  },
-
   [types.REQUEST_DELETE_VARIABLE](state) {
     state.isDeleting = true;
   },
@@ -68,11 +64,6 @@ export default {
     state.environments.unshift(__('All environments'));
   },
 
-  [types.RECEIVE_ENVIRONMENTS_ERROR](state, error) {
-    state.isLoading = false;
-    state.error = error;
-  },
-
   [types.VARIABLE_BEING_EDITED](state, variable) {
     state.variableBeingEdited = variable;
   },

app/assets/javascripts/ci_variable_list/store/utils.js

 import { __ } from '~/locale';
 
-const variableTypeHandler = type => (type === 'Variable' ? 'env_var' : 'file');
+const variableType = 'env_var';
+const fileType = 'file';
+
+const variableTypeHandler = type => (type === 'Variable' ? variableType : fileType);
 
 export const prepareDataForDisplay = variables => {
   const variablesToDisplay = [];
   variables.forEach(variable => {
     const variableCopy = variable;
-    if (variableCopy.variable_type === 'env_var') {
+    if (variableCopy.variable_type === variableType) {
       variableCopy.variable_type = __('Variable');
     } else {
       variableCopy.variable_type = __('File');
@@ -38,10 +41,4 @@ export const prepareDataForApi = (variable, destroy = false) => {
   return variableCopy;
 };
 
-export const prepareEnvironments = environments => {
-  const environmentNames = [];
-  environments.forEach(environment => {
-    environmentNames.push(environment.name);
-  });
-  return environmentNames;
-};
+export const prepareEnvironments = environments => environments.map(e => e.name);

locale/gitlab.pot

@@ -19359,6 +19359,12 @@ msgstr ""
 msgid "There was an error fetching the Designs"
 msgstr ""
 
+msgid "There was an error fetching the environments information."
+msgstr ""
+
+msgid "There was an error fetching the variables."
+msgstr ""
+
 msgid "There was an error fetching value stream analytics stages."
 msgstr ""
 

spec/frontend/ci_variable_list/services/mock_data.js

@@ -29,6 +29,48 @@ export default {
     },
   ],
 
+  mockVariablesApi: [
+    {
+      environment_scope: '*',
+      id: 113,
+      key: 'test_var',
+      masked: false,
+      protected: false,
+      value: 'test_val',
+      variable_type: 'env_var',
+    },
+    {
+      environment_scope: '*',
+      id: 114,
+      key: 'test_var_2',
+      masked: false,
+      protected: false,
+      value: 'test_val_2',
+      variable_type: 'file',
+    },
+  ],
+
+  mockVariablesDisplay: [
+    {
+      environment_scope: 'All environments',
+      id: 113,
+      key: 'test_var',
+      masked: false,
+      protected: false,
+      value: 'test_val',
+      variable_type: 'Variable',
+    },
+    {
+      environment_scope: 'All environments',
+      id: 114,
+      key: 'test_var_2',
+      masked: false,
+      protected: false,
+      value: 'test_val_2',
+      variable_type: 'File',
+    },
+  ],
+
   mockEnvironments: [
     {
       id: 28,

spec/frontend/ci_variable_list/store/actions_spec.js

@@ -2,6 +2,7 @@ import Api from '~/api';
 import MockAdapter from 'axios-mock-adapter';
 import testAction from 'helpers/vuex_action_helper';
 import axios from '~/lib/utils/axios_utils';
+import createFlash from '~/flash';
 import getInitialState from '~/ci_variable_list/store/state';
 import * as actions from '~/ci_variable_list/store/actions';
 import * as types from '~/ci_variable_list/store/mutation_types';
@@ -9,6 +10,7 @@ import mockData from '../services/mock_data';
 import { prepareDataForDisplay, prepareEnvironments } from '~/ci_variable_list/store/utils';
 
 jest.mock('~/api.js');
+jest.mock('~/flash.js');
 
 describe('CI variable list store actions', () => {
   let mock;
@@ -23,6 +25,7 @@ describe('CI variable list store actions', () => {
     variable_type: 'env_var',
     _destory: true,
   };
+  const payloadError = new Error('Request failed with status code 500');
 
   beforeEach(() => {
     mock = new MockAdapter(axios);
@@ -91,6 +94,28 @@ describe('CI variable list store actions', () => {
         },
       );
     });
+
+    it('should show flash error and set error in state on delete failure', done => {
+      mock.onPatch(state.endpoint).reply(500, '');
+
+      testAction(
+        actions.deleteVariable,
+        mockVariable,
+        state,
+        [],
+        [
+          { type: 'requestDeleteVariable' },
+          {
+            type: 'receiveDeleteVariableError',
+            payload: payloadError,
+          },
+        ],
+        () => {
+          expect(createFlash).toHaveBeenCalled();
+          done();
+        },
+      );
+    });
   });
 
   describe('updateVariable', () => {
@@ -112,6 +137,28 @@ describe('CI variable list store actions', () => {
         },
       );
     });
+
+    it('should show flash error and set error in state on update failure', done => {
+      mock.onPatch(state.endpoint).reply(500, '');
+
+      testAction(
+        actions.updateVariable,
+        mockVariable,
+        state,
+        [],
+        [
+          { type: 'requestUpdateVariable' },
+          {
+            type: 'receiveUpdateVariableError',
+            payload: payloadError,
+          },
+        ],
+        () => {
+          expect(createFlash).toHaveBeenCalled();
+          done();
+        },
+      );
+    });
   });
 
   describe('addVariable', () => {
@@ -133,6 +180,28 @@ describe('CI variable list store actions', () => {
         },
       );
     });
+
+    it('should show flash error and set error in state on add failure', done => {
+      mock.onPatch(state.endpoint).reply(500, '');
+
+      testAction(
+        actions.addVariable,
+        {},
+        state,
+        [],
+        [
+          { type: 'requestAddVariable' },
+          {
+            type: 'receiveAddVariableError',
+            payload: payloadError,
+          },
+        ],
+        () => {
+          expect(createFlash).toHaveBeenCalled();
+          done();
+        },
+      );
+    });
   });
 
   describe('fetchVariables', () => {
@@ -156,6 +225,15 @@ describe('CI variable list store actions', () => {
         },
       );
     });
+
+    it('should show flash error and set error in state on fetch variables failure', done => {
+      mock.onGet(state.endpoint).reply(500);
+
+      testAction(actions.fetchVariables, {}, state, [], [{ type: 'requestVariables' }], () => {
+        expect(createFlash).toHaveBeenCalledWith('There was an error fetching the variables.');
+        done();
+      });
+    });
   });
 
   describe('fetchEnvironments', () => {
@@ -179,5 +257,23 @@ describe('CI variable list store actions', () => {
         },
       );
     });
+
+    it('should show flash error and set error in state on fetch environments failure', done => {
+      Api.environments = jest.fn().mockRejectedValue();
+
+      testAction(
+        actions.fetchEnvironments,
+        {},
+        state,
+        [],
+        [{ type: 'requestEnvironments' }],
+        () => {
+          expect(createFlash).toHaveBeenCalledWith(
+            'There was an error fetching the environments information.',
+          );
+          done();
+        },
+      );
+    });
   });
 });

spec/frontend/ci_variable_list/store/utils_spec.js

+import {
+  prepareDataForDisplay,
+  prepareEnvironments,
+  prepareDataForApi,
+} from '~/ci_variable_list/store/utils';
+import mockData from '../services/mock_data';
+
+describe('CI variables store utils', () => {
+  it('prepares ci variables for display', () => {
+    expect(prepareDataForDisplay(mockData.mockVariablesApi)).toStrictEqual(
+      mockData.mockVariablesDisplay,
+    );
+  });
+
+  it('prepares single ci variable for api', () => {
+    expect(prepareDataForApi(mockData.mockVariablesDisplay[0])).toStrictEqual({
+      environment_scope: '*',
+      id: 113,
+      key: 'test_var',
+      masked: false,
+      protected: false,
+      value: 'test_val',
+      variable_type: 'env_var',
+    });
+
+    expect(prepareDataForApi(mockData.mockVariablesDisplay[1])).toStrictEqual({
+      environment_scope: '*',
+      id: 114,
+      key: 'test_var_2',
+      masked: false,
+      protected: false,
+      value: 'test_val_2',
+      variable_type: 'file',
+    });
+  });
+
+  it('prepares single ci variable for delete', () => {
+    expect(prepareDataForApi(mockData.mockVariablesDisplay[0], true)).toHaveProperty(
+      '_destroy',
+      true,
+    );
+  });
+
+  it('prepares environments for display', () => {
+    expect(prepareEnvironments(mockData.mockEnvironments)).toStrictEqual(['staging', 'production']);
+  });
+});