Merge branch 'refactor-ci-variable-modal' into 'master'

Refactor CI variable modal

Closes #214638

See merge request gitlab-org/gitlab!35353

app/assets/javascripts/ci_variable_list/components/ci_variable_modal.vue

@@ -16,6 +16,7 @@ import {
 } from '@gitlab/ui';
 import Cookies from 'js-cookie';
 import { mapActions, mapState } from 'vuex';
+import { mapComputed } from '~/vuex_shared/bindings';
 import { __ } from '~/locale';
 import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
 import {
@@ -30,6 +31,9 @@ import CiEnvironmentsDropdown from './ci_environments_dropdown.vue';
 
 export default {
   modalId: ADD_CI_VARIABLE_MODAL_ID,
+  tokens: awsTokens,
+  tokenList: awsTokenList,
+  awsTipMessage: AWS_TIP_MESSAGE,
   components: {
     CiEnvironmentsDropdown,
     CiKeyField,
@@ -48,9 +52,6 @@ export default {
     GlSprintf,
   },
   mixins: [glFeatureFlagsMixin()],
-  tokens: awsTokens,
-  tokenList: awsTokenList,
-  awsTipMessage: AWS_TIP_MESSAGE,
   data() {
     return {
       isTipDismissed: Cookies.get(AWS_TIP_DISMISSED_COOKIE_NAME) === 'true',
@@ -74,22 +75,34 @@ export default {
       'protectedEnvironmentVariablesLink',
       'maskedEnvironmentVariablesLink',
     ]),
+    ...mapComputed(
+      [
+        { key: 'key', updateFn: 'updateVariableKey' },
+        { key: 'secret_value', updateFn: 'updateVariableValue' },
+        { key: 'variable_type', updateFn: 'updateVariableType' },
+        { key: 'environment_scope', updateFn: 'setEnvironmentScope' },
+        { key: 'protected_variable', updateFn: 'updateVariableProtected' },
+        { key: 'masked', updateFn: 'updateVariableMasked' },
+      ],
+      false,
+      'variable',
+    ),
     isTipVisible() {
-      return !this.isTipDismissed && AWS_TOKEN_CONSTANTS.includes(this.variableData.key);
+      return !this.isTipDismissed && AWS_TOKEN_CONSTANTS.includes(this.variable.key);
     },
     canSubmit() {
       return (
         this.variableValidationState &&
-        this.variableData.key !== '' &&
-        this.variableData.secret_value !== ''
+        this.variable.key !== '' &&
+        this.variable.secret_value !== ''
       );
     },
     canMask() {
       const regex = RegExp(this.maskableRegex);
-      return regex.test(this.variableData.secret_value);
+      return regex.test(this.variable.secret_value);
     },
     displayMaskedError() {
-      return !this.canMask && this.variableData.masked;
+      return !this.canMask && this.variable.masked;
     },
     maskedState() {
       if (this.displayMaskedError) {
@@ -97,9 +110,6 @@ export default {
       }
       return true;
     },
-    variableData() {
-      return this.variableBeingEdited || this.variable;
-    },
     modalActionText() {
       return this.variableBeingEdited ? __('Update variable') : __('Add variable');
     },
@@ -107,7 +117,7 @@ export default {
       return this.displayMaskedError ? __('This variable can not be masked.') : '';
     },
     tokenValidationFeedback() {
-      const tokenSpecificFeedback = this.$options.tokens?.[this.variableData.key]?.invalidMessage;
+      const tokenSpecificFeedback = this.$options.tokens?.[this.variable.key]?.invalidMessage;
       if (!this.tokenValidationState && tokenSpecificFeedback) {
         return tokenSpecificFeedback;
       }
@@ -119,10 +129,10 @@ export default {
         return true;
       }
 
-      const validator = this.$options.tokens?.[this.variableData.key]?.validation;
+      const validator = this.$options.tokens?.[this.variable.key]?.validation;
 
       if (validator) {
-        return validator(this.variableData.secret_value);
+        return validator(this.variable.secret_value);
       }
 
       return true;
@@ -131,14 +141,7 @@ export default {
       return `${this.tokenValidationFeedback} ${this.maskedFeedback}`;
     },
     variableValidationState() {
-      if (
-        this.variableData.secret_value === '' ||
-        (this.tokenValidationState && this.maskedState)
-      ) {
-        return true;
-      }
-
-      return false;
+      return this.variable.secret_value === '' || (this.tokenValidationState && this.maskedState);
     },
   },
   methods: {
@@ -160,7 +163,7 @@ export default {
       this.isTipDismissed = true;
     },
     deleteVarAndClose() {
-      this.deleteVariable(this.variableBeingEdited);
+      this.deleteVariable();
       this.hideModal();
     },
     hideModal() {
@@ -169,14 +172,14 @@ export default {
     resetModalHandler() {
       if (this.variableBeingEdited) {
         this.resetEditing();
-      } else {
-        this.clearModal();
       }
+
+      this.clearModal();
       this.resetSelectedEnvironment();
     },
     updateOrAddVariable() {
       if (this.variableBeingEdited) {
-        this.updateVariable(this.variableBeingEdited);
+        this.updateVariable();
       } else {
         this.addVariable();
       }
@@ -204,14 +207,14 @@ export default {
     <form>
       <ci-key-field
         v-if="glFeatures.ciKeyAutocomplete"
-        v-model="variableData.key"
+        v-model="key"
         :token-list="$options.tokenList"
       />
 
       <gl-form-group v-else :label="__('Key')" label-for="ci-variable-key">
         <gl-form-input
           id="ci-variable-key"
-          v-model="variableData.key"
+          v-model="key"
           data-qa-selector="ci_variable_key_field"
         />
       </gl-form-group>
@@ -225,7 +228,7 @@ export default {
         <gl-form-textarea
           id="ci-variable-value"
           ref="valueField"
-          v-model="variableData.secret_value"
+          v-model="secret_value"
           :state="variableValidationState"
           rows="3"
           max-rows="6"
@@ -241,11 +244,7 @@ export default {
           class="w-50 append-right-15"
           :class="{ 'w-100': isGroup }"
         >
-          <gl-form-select
-            id="ci-variable-type"
-            v-model="variableData.variable_type"
-            :options="typeOptions"
-          />
+          <gl-form-select id="ci-variable-type" v-model="variable_type" :options="typeOptions" />
         </gl-form-group>
 
         <gl-form-group
@@ -256,7 +255,7 @@ export default {
         >
           <ci-environments-dropdown
             class="w-100"
-            :value="variableData.environment_scope"
+            :value="environment_scope"
             @selectEnvironment="setEnvironmentScope"
             @createClicked="addWildCardScope"
           />
@@ -264,7 +263,7 @@ export default {
       </div>
 
       <gl-form-group :label="__('Flags')" label-for="ci-variable-flags">
-        <gl-form-checkbox v-model="variableData.protected" class="mb-0">
+        <gl-form-checkbox v-model="protected_variable" class="mb-0">
           {{ __('Protect variable') }}
           <gl-link target="_blank" :href="protectedEnvironmentVariablesLink">
             <gl-icon name="question" :size="12" />
@@ -276,7 +275,7 @@ export default {
 
         <gl-form-checkbox
           ref="masked-ci-variable"
-          v-model="variableData.masked"
+          v-model="masked"
           data-qa-selector="ci_variable_masked_checkbox"
         >
           {{ __('Mask variable') }}

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

@@ -65,10 +65,10 @@ export const receiveUpdateVariableError = ({ commit }, error) => {
   commit(types.RECEIVE_UPDATE_VARIABLE_ERROR, error);
 };
 
-export const updateVariable = ({ state, dispatch }, variable) => {
+export const updateVariable = ({ state, dispatch }) => {
   dispatch('requestUpdateVariable');
 
-  const updatedVariable = prepareDataForApi(variable);
+  const updatedVariable = prepareDataForApi(state.variable);
   updatedVariable.secrect_value = updateVariable.value;
 
   return axios
@@ -121,13 +121,13 @@ export const receiveDeleteVariableError = ({ commit }, error) => {
   commit(types.RECEIVE_DELETE_VARIABLE_ERROR, error);
 };
 
-export const deleteVariable = ({ dispatch, state }, variable) => {
+export const deleteVariable = ({ dispatch, state }) => {
   dispatch('requestDeleteVariable');
 
   const destroy = true;
 
   return axios
-    .patch(state.endpoint, { variables_attributes: [prepareDataForApi(variable, destroy)] })
+    .patch(state.endpoint, { variables_attributes: [prepareDataForApi(state.variable, destroy)] })
     .then(() => {
       dispatch('receiveDeleteVariableSuccess');
       dispatch('fetchVariables');
@@ -176,3 +176,23 @@ export const resetSelectedEnvironment = ({ commit }) => {
 export const setSelectedEnvironment = ({ commit }, environment) => {
   commit(types.SET_SELECTED_ENVIRONMENT, environment);
 };
+
+export const updateVariableKey = ({ commit }, { key }) => {
+  commit(types.UPDATE_VARIABLE_KEY, key);
+};
+
+export const updateVariableValue = ({ commit }, { secret_value }) => {
+  commit(types.UPDATE_VARIABLE_VALUE, secret_value);
+};
+
+export const updateVariableType = ({ commit }, { variable_type }) => {
+  commit(types.UPDATE_VARIABLE_TYPE, variable_type);
+};
+
+export const updateVariableProtected = ({ commit }, { protected_variable }) => {
+  commit(types.UPDATE_VARIABLE_PROTECTED, protected_variable);
+};
+
+export const updateVariableMasked = ({ commit }, { masked }) => {
+  commit(types.UPDATE_VARIABLE_MASKED, masked);
+};

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

@@ -25,3 +25,9 @@ export const SET_ENVIRONMENT_SCOPE = 'SET_ENVIRONMENT_SCOPE';
 export const ADD_WILD_CARD_SCOPE = 'ADD_WILD_CARD_SCOPE';
 export const RESET_SELECTED_ENVIRONMENT = 'RESET_SELECTED_ENVIRONMENT';
 export const SET_SELECTED_ENVIRONMENT = 'SET_SELECTED_ENVIRONMENT';
+
+export const UPDATE_VARIABLE_KEY = 'UPDATE_VARIABLE_KEY';
+export const UPDATE_VARIABLE_VALUE = 'UPDATE_VARIABLE_VALUE';
+export const UPDATE_VARIABLE_TYPE = 'UPDATE_VARIABLE_TYPE';
+export const UPDATE_VARIABLE_PROTECTED = 'UPDATE_VARIABLE_PROTECTED';
+export const UPDATE_VARIABLE_MASKED = 'UPDATE_VARIABLE_MASKED';

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

@@ -65,7 +65,8 @@ export default {
   },
 
   [types.VARIABLE_BEING_EDITED](state, variable) {
-    state.variableBeingEdited = variable;
+    state.variableBeingEdited = true;
+    state.variable = variable;
   },
 
   [types.CLEAR_MODAL](state) {
@@ -80,16 +81,12 @@ export default {
   },
 
   [types.RESET_EDITING](state) {
-    state.variableBeingEdited = null;
+    state.variableBeingEdited = false;
     state.showInputValue = false;
   },
 
   [types.SET_ENVIRONMENT_SCOPE](state, environment) {
-    if (state.variableBeingEdited) {
-      state.variableBeingEdited.environment_scope = environment;
-    } else {
     state.variable.environment_scope = environment;
-    }
   },
 
   [types.ADD_WILD_CARD_SCOPE](state, environment) {
@@ -108,4 +105,24 @@ export default {
   [types.SET_VARIABLE_PROTECTED](state) {
     state.variable.protected = true;
   },
+
+  [types.UPDATE_VARIABLE_KEY](state, key) {
+    state.variable.key = key;
+  },
+
+  [types.UPDATE_VARIABLE_VALUE](state, value) {
+    state.variable.secret_value = value;
+  },
+
+  [types.UPDATE_VARIABLE_TYPE](state, type) {
+    state.variable.variable_type = type;
+  },
+
+  [types.UPDATE_VARIABLE_PROTECTED](state, bool) {
+    state.variable.protected_variable = bool;
+  },
+
+  [types.UPDATE_VARIABLE_MASKED](state, bool) {
+    state.variable.masked = bool;
+  },
 };

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

@@ -12,7 +12,7 @@ export default () => ({
     variable_type: displayText.variableText,
     key: '',
     secret_value: '',
-    protected: false,
+    protected_variable: false,
     masked: false,
     environment_scope: displayText.allEnvironmentsText,
   },
@@ -21,6 +21,6 @@ export default () => ({
   error: null,
   environments: [],
   typeOptions: [displayText.variableText, displayText.fileText],
-  variableBeingEdited: null,
+  variableBeingEdited: false,
   selectedEnvironment: '',
 });

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

@@ -18,6 +18,7 @@ export const prepareDataForDisplay = variables => {
     if (variableCopy.environment_scope === types.allEnvironmentsType) {
       variableCopy.environment_scope = displayText.allEnvironmentsText;
     }
+    variableCopy.protected_variable = variableCopy.protected;
     variablesToDisplay.push(variableCopy);
   });
   return variablesToDisplay;
@@ -25,7 +26,8 @@ export const prepareDataForDisplay = variables => {
 
 export const prepareDataForApi = (variable, destroy = false) => {
   const variableCopy = cloneDeep(variable);
-  variableCopy.protected = variableCopy.protected.toString();
+  variableCopy.protected = variableCopy.protected_variable.toString();
+  delete variableCopy.protected_variable;
   variableCopy.masked = variableCopy.masked.toString();
   variableCopy.variable_type = variableTypeHandler(variableCopy.variable_type);
   if (variableCopy.environment_scope === displayText.allEnvironmentsText) {

spec/frontend/ci_variable_list/components/ci_variable_modal_spec.js

@@ -159,10 +159,7 @@ describe('Ci variable modal', () => {
 
     it('Update variable button dispatches updateVariable with correct variable', () => {
       addOrUpdateButton(2).vm.$emit('click');
-      expect(store.dispatch).toHaveBeenCalledWith(
-        'updateVariable',
-        store.state.variableBeingEdited,
-      );
+      expect(store.dispatch).toHaveBeenCalledWith('updateVariable');
     });
 
     it('Resets the editing state once modal is hidden', () => {
@@ -172,7 +169,7 @@ describe('Ci variable modal', () => {
 
     it('dispatches deleteVariable with correct variable to delete', () => {
       deleteVariableButton().vm.$emit('click');
-      expect(store.dispatch).toHaveBeenCalledWith('deleteVariable', mockData.mockVariables[0]);
+      expect(store.dispatch).toHaveBeenCalledWith('deleteVariable');
     });
   });
 

spec/frontend/ci_variable_list/services/mock_data.js

@@ -42,6 +42,7 @@ export default {
       key: 'test_var',
       masked: false,
       protected: false,
+      protected_variable: false,
       secret_value: 'test_val',
       value: 'test_val',
       variable_type: 'Variable',
@@ -52,6 +53,7 @@ export default {
       key: 'test_var_2',
       masked: false,
       protected: false,
+      protected_variable: false,
       secret_value: 'test_val_2',
       value: 'test_val_2',
       variable_type: 'File',

spec/frontend/ci_variable_list/store/actions_spec.js

@@ -91,7 +91,7 @@ describe('CI variable list store actions', () => {
 
       testAction(
         actions.deleteVariable,
-        mockVariable,
+        {},
         state,
         [],
         [
@@ -110,7 +110,7 @@ describe('CI variable list store actions', () => {
 
       testAction(
         actions.deleteVariable,
-        mockVariable,
+        {},
         state,
         [],
         [
@@ -134,7 +134,7 @@ describe('CI variable list store actions', () => {
 
       testAction(
         actions.updateVariable,
-        mockVariable,
+        {},
         state,
         [],
         [
@@ -286,4 +286,66 @@ describe('CI variable list store actions', () => {
       );
     });
   });
+
+  describe('Update variable values', () => {
+    it('updateVariableKey', () => {
+      testAction(
+        actions.updateVariableKey,
+        { key: mockVariable.key },
+        {},
+        [
+          {
+            type: types.UPDATE_VARIABLE_KEY,
+            payload: mockVariable.key,
+          },
+        ],
+        [],
+      );
+    });
+
+    it('updateVariableValue', () => {
+      testAction(
+        actions.updateVariableValue,
+        { secret_value: mockVariable.value },
+        {},
+        [
+          {
+            type: types.UPDATE_VARIABLE_VALUE,
+            payload: mockVariable.value,
+          },
+        ],
+        [],
+      );
+    });
+
+    it('updateVariableType', () => {
+      testAction(
+        actions.updateVariableType,
+        { variable_type: mockVariable.variable_type },
+        {},
+        [{ type: types.UPDATE_VARIABLE_TYPE, payload: mockVariable.variable_type }],
+        [],
+      );
+    });
+
+    it('updateVariableProtected', () => {
+      testAction(
+        actions.updateVariableProtected,
+        { protected_variable: mockVariable.protected },
+        {},
+        [{ type: types.UPDATE_VARIABLE_PROTECTED, payload: mockVariable.protected }],
+        [],
+      );
+    });
+
+    it('updateVariableMasked', () => {
+      testAction(
+        actions.updateVariableMasked,
+        { masked: mockVariable.masked },
+        {},
+        [{ type: types.UPDATE_VARIABLE_MASKED, payload: mockVariable.masked }],
+        [],
+      );
+    });
+  });
 });

spec/frontend/ci_variable_list/store/mutations_spec.js

@@ -4,15 +4,6 @@ import * as types from '~/ci_variable_list/store/mutation_types';
 
 describe('CI variable list mutations', () => {
   let stateCopy;
-  const variableBeingEdited = {
-    environment_scope: '*',
-    id: 63,
-    key: 'test_var',
-    masked: false,
-    protected: false,
-    value: 'test_val',
-    variable_type: 'env_var',
-  };
 
   beforeEach(() => {
     stateCopy = state();
@@ -29,18 +20,18 @@ describe('CI variable list mutations', () => {
   });
 
   describe('VARIABLE_BEING_EDITED', () => {
-    it('should set variable that is being edited', () => {
-      mutations[types.VARIABLE_BEING_EDITED](stateCopy, variableBeingEdited);
+    it('should set the variable that is being edited', () => {
+      mutations[types.VARIABLE_BEING_EDITED](stateCopy);
 
-      expect(stateCopy.variableBeingEdited).toEqual(variableBeingEdited);
+      expect(stateCopy.variableBeingEdited).toBe(true);
     });
   });
 
   describe('RESET_EDITING', () => {
-    it('should reset variableBeingEdited to null', () => {
+    it('should reset variableBeingEdited to false', () => {
       mutations[types.RESET_EDITING](stateCopy);
 
-      expect(stateCopy.variableBeingEdited).toEqual(null);
+      expect(stateCopy.variableBeingEdited).toBe(false);
     });
   });
 
@@ -74,15 +65,7 @@ describe('CI variable list mutations', () => {
   describe('SET_ENVIRONMENT_SCOPE', () => {
     const environment = 'production';
 
-    it('should set scope to variable being updated if updating variable', () => {
-      stateCopy.variableBeingEdited = variableBeingEdited;
-
-      mutations[types.SET_ENVIRONMENT_SCOPE](stateCopy, environment);
-
-      expect(stateCopy.variableBeingEdited.environment_scope).toBe('production');
-    });
-
-    it('should set scope to variable if adding new variable', () => {
+    it('should set environment scope on variable', () => {
       mutations[types.SET_ENVIRONMENT_SCOPE](stateCopy, environment);
 
       expect(stateCopy.variable.environment_scope).toBe('production');
@@ -105,4 +88,49 @@ describe('CI variable list mutations', () => {
       expect(stateCopy.variable.protected).toBe(true);
     });
   });
+
+  describe('UPDATE_VARIABLE_KEY', () => {
+    it('should update variable key value', () => {
+      const key = 'new_var';
+      mutations[types.UPDATE_VARIABLE_KEY](stateCopy, key);
+
+      expect(stateCopy.variable.key).toBe(key);
+    });
+  });
+
+  describe('UPDATE_VARIABLE_VALUE', () => {
+    it('should update variable value', () => {
+      const value = 'variable_value';
+      mutations[types.UPDATE_VARIABLE_VALUE](stateCopy, value);
+
+      expect(stateCopy.variable.secret_value).toBe(value);
+    });
+  });
+
+  describe('UPDATE_VARIABLE_TYPE', () => {
+    it('should update variable type value', () => {
+      const type = 'File';
+      mutations[types.UPDATE_VARIABLE_TYPE](stateCopy, type);
+
+      expect(stateCopy.variable.variable_type).toBe(type);
+    });
+  });
+
+  describe('UPDATE_VARIABLE_PROTECTED', () => {
+    it('should update variable protected value', () => {
+      const protectedValue = true;
+      mutations[types.UPDATE_VARIABLE_PROTECTED](stateCopy, protectedValue);
+
+      expect(stateCopy.variable.protected_variable).toBe(protectedValue);
+    });
+  });
+
+  describe('UPDATE_VARIABLE_MASKED', () => {
+    it('should update variable masked value', () => {
+      const masked = true;
+      mutations[types.UPDATE_VARIABLE_MASKED](stateCopy, masked);
+
+      expect(stateCopy.variable.masked).toBe(masked);
+    });
+  });
 });