Merge branch '326809-refactor-manual-vars-form' into 'master'

Refactor manual variables form

See merge request gitlab-org/gitlab!73013

app/assets/javascripts/jobs/components/manual_variables_form.vue

 <script>
-import { GlButton, GlLink, GlSprintf } from '@gitlab/ui';
+import {
+  GlFormInputGroup,
+  GlInputGroupText,
+  GlFormInput,
+  GlButton,
+  GlLink,
+  GlSprintf,
+} from '@gitlab/ui';
 import { uniqueId } from 'lodash';
 import { mapActions } from 'vuex';
 import { helpPagePath } from '~/helpers/help_page_helper';
@@ -8,6 +15,9 @@ import { s__ } from '~/locale';
 export default {
   name: 'ManualVariablesForm',
   components: {
+    GlFormInputGroup,
+    GlInputGroupText,
+    GlFormInput,
     GlButton,
     GlLink,
     GlSprintf,
@@ -32,6 +42,9 @@ export default {
     value: 'value',
   },
   i18n: {
+    header: s__('CiVariables|Variables'),
+    keyLabel: s__('CiVariables|Key'),
+    valueLabel: s__('CiVariables|Value'),
     keyPlaceholder: s__('CiVariables|Input variable key'),
     valuePlaceholder: s__('CiVariables|Input variable value'),
     formHelpText: s__(
@@ -40,9 +53,13 @@ export default {
   },
   data() {
     return {
-      variables: [],
+      variables: [
+        {
           key: '',
           secretValue: '',
+          id: uniqueId(),
+        },
+      ],
       triggerBtnDisabled: false,
     };
   },
@@ -50,40 +67,32 @@ export default {
     variableSettings() {
       return helpPagePath('ci/variables/index', { anchor: 'add-a-cicd-variable-to-a-project' });
     },
-  },
-  watch: {
-    key(newVal) {
-      this.handleValueChange(newVal, this.$options.inputTypes.key);
-    },
-    secretValue(newVal) {
-      this.handleValueChange(newVal, this.$options.inputTypes.value);
+    preparedVariables() {
+      // we need to ensure no empty variables are passed to the API
+      // and secretValue should be snake_case when passed to the API
+      return this.variables
+        .filter((variable) => variable.key !== '')
+        .map(({ key, secretValue }) => ({ key, secret_value: secretValue }));
     },
   },
   methods: {
     ...mapActions(['triggerManualJob']),
-    handleValueChange(newValue, type) {
-      if (newValue !== '') {
-        this.createNewVariable(type);
-        this.resetForm();
-      }
+    canRemove(index) {
+      return index < this.variables.length - 1;
     },
-    createNewVariable(type) {
-      const newVariable = {
-        key: this.key,
-        secret_value: this.secretValue,
-        id: uniqueId(),
-      };
+    addEmptyVariable() {
+      const lastVar = this.variables[this.variables.length - 1];
 
-      this.variables.push(newVariable);
+      if (lastVar.key === '') {
+        return;
+      }
 
-      return this.$nextTick().then(() => {
-        this.$refs[`${this.$options.inputTypes[type]}-${newVariable.id}`][0].focus();
+      this.variables.push({
+        key: '',
+        secret_value: '',
+        id: uniqueId(),
       });
     },
-    resetForm() {
-      this.key = '';
-      this.secretValue = '';
-    },
     deleteVariable(id) {
       this.variables.splice(
         this.variables.findIndex((el) => el.id === id),
@@ -93,93 +102,71 @@ export default {
     trigger() {
       this.triggerBtnDisabled = true;
 
-      this.triggerManualJob(this.variables);
+      this.triggerManualJob(this.preparedVariables);
     },
   },
 };
 </script>
 <template>
-  <div class="col-12" data-testid="manual-vars-form">
-    <label>{{ s__('CiVariables|Variables') }}</label>
-
-    <div class="ci-table">
-      <div class="gl-responsive-table-row table-row-header pb-0 pt-0 border-0" role="row">
-        <div class="table-section section-50" role="rowheader">{{ s__('CiVariables|Key') }}</div>
-        <div class="table-section section-50" role="rowheader">{{ s__('CiVariables|Value') }}</div>
-      </div>
+  <div class="row gl-justify-content-center">
+    <div class="col-10" data-testid="manual-vars-form">
+      <label>{{ $options.i18n.header }}</label>
 
       <div
-        v-for="variable in variables"
+        v-for="(variable, index) in variables"
         :key="variable.id"
-        class="gl-responsive-table-row"
+        class="gl-display-flex gl-align-items-center gl-mb-4"
         data-testid="ci-variable-row"
       >
-        <div class="table-section section-50">
-          <div class="table-mobile-header" role="rowheader">{{ s__('Pipeline|Key') }}</div>
-          <div class="table-mobile-content gl-mr-3">
-            <input
+        <gl-form-input-group class="gl-mr-4 gl-flex-grow-1">
+          <template #prepend>
+            <gl-input-group-text>
+              {{ $options.i18n.keyLabel }}
+            </gl-input-group-text>
+          </template>
+          <gl-form-input
             :ref="`${$options.inputTypes.key}-${variable.id}`"
             v-model="variable.key"
             :placeholder="$options.i18n.keyPlaceholder"
-              class="ci-variable-body-item form-control"
             data-testid="ci-variable-key"
+            @change="addEmptyVariable"
           />
-          </div>
-        </div>
+        </gl-form-input-group>
 
-        <div class="table-section section-50">
-          <div class="table-mobile-header" role="rowheader">{{ s__('Pipeline|Value') }}</div>
-          <div class="table-mobile-content gl-mr-3">
-            <input
+        <gl-form-input-group class="gl-flex-grow-2">
+          <template #prepend>
+            <gl-input-group-text>
+              {{ $options.i18n.valueLabel }}
+            </gl-input-group-text>
+          </template>
+          <gl-form-input
             :ref="`${$options.inputTypes.value}-${variable.id}`"
-              v-model="variable.secret_value"
+            v-model="variable.secretValue"
             :placeholder="$options.i18n.valuePlaceholder"
-              class="ci-variable-body-item form-control"
             data-testid="ci-variable-value"
           />
-          </div>
-        </div>
+        </gl-form-input-group>
+
+        <!-- delete variable button placeholder to not break flex layout  -->
+        <div
+          v-if="!canRemove(index)"
+          class="gl-w-7 gl-mr-3"
+          data-testid="delete-variable-btn-placeholder"
+        ></div>
 
-        <div class="table-section section-10">
-          <div class="table-mobile-header" role="rowheader"></div>
-          <div class="table-mobile-content justify-content-end">
         <gl-button
+          v-if="canRemove(index)"
+          class="gl-flex-grow-0 gl-flex-basis-0"
           category="tertiary"
+          variant="danger"
           icon="clear"
           :aria-label="__('Delete variable')"
           data-testid="delete-variable-btn"
           @click="deleteVariable(variable.id)"
         />
       </div>
-        </div>
-      </div>
-      <div class="gl-responsive-table-row">
-        <div class="table-section section-50">
-          <div class="table-mobile-header" role="rowheader">{{ s__('Pipeline|Key') }}</div>
-          <div class="table-mobile-content gl-mr-3">
-            <input
-              ref="inputKey"
-              v-model="key"
-              class="js-input-key form-control"
-              :placeholder="$options.i18n.keyPlaceholder"
-            />
-          </div>
-        </div>
 
-        <div class="table-section section-50">
-          <div class="table-mobile-header" role="rowheader">{{ s__('Pipeline|Value') }}</div>
-          <div class="table-mobile-content gl-mr-3">
-            <input
-              ref="inputSecretValue"
-              v-model="secretValue"
-              class="ci-variable-body-item form-control"
-              :placeholder="$options.i18n.valuePlaceholder"
-            />
-          </div>
-        </div>
-      </div>
-    </div>
-    <div class="gl-text-center gl-mt-3">
+      <div class="gl-text-center gl-mt-5">
         <gl-sprintf :message="$options.i18n.formHelpText">
           <template #link="{ content }">
             <gl-link :href="variableSettings" target="_blank">
@@ -188,8 +175,9 @@ export default {
           </template>
         </gl-sprintf>
       </div>
-    <div class="d-flex justify-content-center">
+      <div class="gl-display-flex gl-justify-content-center gl-mt-5">
         <gl-button
+          class="gl-mt-5"
           variant="info"
           category="primary"
           :aria-label="__('Trigger manual job')"
@@ -201,4 +189,5 @@ export default {
         </gl-button>
       </div>
     </div>
+  </div>
 </template>

locale/gitlab.pot

@@ -25525,9 +25525,6 @@ msgstr ""
 msgid "Pipeline|In progress"
 msgstr ""
 
-msgid "Pipeline|Key"
-msgstr ""
-
 msgid "Pipeline|Manual"
 msgstr ""
 
@@ -25618,9 +25615,6 @@ msgstr ""
 msgid "Pipeline|Triggerer"
 msgstr ""
 
-msgid "Pipeline|Value"
-msgstr ""
-
 msgid "Pipeline|Variables"
 msgstr ""
 

spec/features/projects/jobs/user_triggers_manual_job_with_variables_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'User triggers manual job with variables', :js do
+  let(:user) { create(:user) }
+  let(:user_access_level) { :developer }
+  let(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:pipeline) { create(:ci_empty_pipeline, project: project, sha: project.commit.sha, ref: 'master') }
+  let!(:build) { create(:ci_build, :manual, pipeline: pipeline) }
+
+  before do
+    project.add_maintainer(user)
+    project.enable_ci
+
+    sign_in(user)
+
+    visit(project_job_path(project, build))
+  end
+
+  it 'passes values correctly' do
+    page.within(find("[data-testid='ci-variable-row']")) do
+      find("[data-testid='ci-variable-key']").set('key_name')
+      find("[data-testid='ci-variable-value']").set('key_value')
+    end
+
+    find("[data-testid='trigger-manual-job-btn']").click
+
+    wait_for_requests
+
+    expect(build.job_variables.as_json).to contain_exactly(
+      hash_including('key' => 'key_name', 'value' => 'key_value'))
+  end
+end

spec/frontend/jobs/components/manual_variables_form_spec.js

 import { GlSprintf, GlLink } from '@gitlab/ui';
-import { createLocalVue, mount, shallowMount } from '@vue/test-utils';
-import Vue from 'vue';
+import { createLocalVue, mount } from '@vue/test-utils';
+import Vue, { nextTick } from 'vue';
 import Vuex from 'vuex';
 import { extendedWrapper } from 'helpers/vue_test_utils_helper';
-import Form from '~/jobs/components/manual_variables_form.vue';
+import ManualVariablesForm from '~/jobs/components/manual_variables_form.vue';
 
 const localVue = createLocalVue();
 
@@ -21,7 +21,7 @@ describe('Manual Variables Form', () => {
     },
   };
 
-  const createComponent = ({ props = {}, mountFn = shallowMount } = {}) => {
+  const createComponent = (props = {}) => {
     store = new Vuex.Store({
       actions: {
         triggerManualJob: jest.fn(),
@@ -29,7 +29,7 @@ describe('Manual Variables Form', () => {
     });
 
     wrapper = extendedWrapper(
-      mountFn(localVue.extend(Form), {
+      mount(localVue.extend(ManualVariablesForm), {
         propsData: { ...requiredProps, ...props },
         localVue,
         store,
@@ -40,80 +40,77 @@ describe('Manual Variables Form', () => {
     );
   };
 
-  const findInputKey = () => wrapper.findComponent({ ref: 'inputKey' });
-  const findInputValue = () => wrapper.findComponent({ ref: 'inputSecretValue' });
   const findHelpText = () => wrapper.findComponent(GlSprintf);
   const findHelpLink = () => wrapper.findComponent(GlLink);
 
   const findTriggerBtn = () => wrapper.findByTestId('trigger-manual-job-btn');
   const findDeleteVarBtn = () => wrapper.findByTestId('delete-variable-btn');
+  const findAllDeleteVarBtns = () => wrapper.findAllByTestId('delete-variable-btn');
+  const findDeleteVarBtnPlaceholder = () => wrapper.findByTestId('delete-variable-btn-placeholder');
   const findCiVariableKey = () => wrapper.findByTestId('ci-variable-key');
+  const findAllCiVariableKeys = () => wrapper.findAllByTestId('ci-variable-key');
   const findCiVariableValue = () => wrapper.findByTestId('ci-variable-value');
   const findAllVariables = () => wrapper.findAllByTestId('ci-variable-row');
 
-  afterEach(() => {
-    wrapper.destroy();
-  });
+  const setCiVariableKey = () => {
+    findCiVariableKey().setValue('new key');
+    findCiVariableKey().vm.$emit('change');
+    nextTick();
+  };
+
+  const setCiVariableKeyByPosition = (position, value) => {
+    findAllCiVariableKeys().at(position).setValue(value);
+    findAllCiVariableKeys().at(position).vm.$emit('change');
+    nextTick();
+  };
 
-  describe('shallowMount', () => {
   beforeEach(() => {
     createComponent();
   });
 
-    it('renders empty form with correct placeholders', () => {
-      expect(findInputKey().attributes('placeholder')).toBe('Input variable key');
-      expect(findInputValue().attributes('placeholder')).toBe('Input variable value');
+  afterEach(() => {
+    wrapper.destroy();
   });
 
-    it('renders help text with provided link', () => {
-      expect(findHelpText().exists()).toBe(true);
-      expect(findHelpLink().attributes('href')).toBe(
-        '/help/ci/variables/index#add-a-cicd-variable-to-a-project',
-      );
-    });
+  it('creates a new variable when user enters a new key value', async () => {
+    expect(findAllVariables()).toHaveLength(1);
 
-    describe('when adding a new variable', () => {
-      it('creates a new variable when user types a new key and resets the form', async () => {
-        await findInputKey().setValue('new key');
+    await setCiVariableKey();
 
-        expect(findAllVariables()).toHaveLength(1);
-        expect(findCiVariableKey().element.value).toBe('new key');
-        expect(findInputKey().attributes('value')).toBe(undefined);
+    expect(findAllVariables()).toHaveLength(2);
   });
 
-      it('creates a new variable when user types a new value and resets the form', async () => {
-        await findInputValue().setValue('new value');
-
+  it('does not create extra empty variables', async () => {
     expect(findAllVariables()).toHaveLength(1);
-        expect(findCiVariableValue().element.value).toBe('new value');
-        expect(findInputValue().attributes('value')).toBe(undefined);
-      });
-    });
-  });
 
-  describe('mount', () => {
-    beforeEach(() => {
-      createComponent({ mountFn: mount });
-    });
+    await setCiVariableKey();
 
-    describe('when deleting a variable', () => {
-      it('removes the variable row', async () => {
-        await wrapper.setData({
-          variables: [
-            {
-              key: 'new key',
-              secret_value: 'value',
-              id: '1',
-            },
-          ],
+    expect(findAllVariables()).toHaveLength(2);
+
+    await setCiVariableKey();
+
+    expect(findAllVariables()).toHaveLength(2);
   });
 
-        findDeleteVarBtn().trigger('click');
+  it('removes the correct variable row', async () => {
+    const variableKeyNameOne = 'key-one';
+    const variableKeyNameThree = 'key-three';
 
-        await wrapper.vm.$nextTick();
+    await setCiVariableKeyByPosition(0, variableKeyNameOne);
 
-        expect(findAllVariables()).toHaveLength(0);
-      });
+    await setCiVariableKeyByPosition(1, 'key-two');
+
+    await setCiVariableKeyByPosition(2, variableKeyNameThree);
+
+    expect(findAllVariables()).toHaveLength(4);
+
+    await findAllDeleteVarBtns().at(1).trigger('click');
+
+    expect(findAllVariables()).toHaveLength(3);
+
+    expect(findAllCiVariableKeys().at(0).element.value).toBe(variableKeyNameOne);
+    expect(findAllCiVariableKeys().at(1).element.value).toBe(variableKeyNameThree);
+    expect(findAllCiVariableKeys().at(2).element.value).toBe('');
   });
 
   it('trigger button is disabled after trigger action', async () => {
@@ -123,5 +120,40 @@ describe('Manual Variables Form', () => {
 
     expect(findTriggerBtn().props('disabled')).toBe(true);
   });
+
+  it('delete variable button should only show when there is more than one variable', async () => {
+    expect(findDeleteVarBtn().exists()).toBe(false);
+
+    await setCiVariableKey();
+
+    expect(findDeleteVarBtn().exists()).toBe(true);
+  });
+
+  it('delete variable button placeholder should only exist when a user cannot remove', async () => {
+    expect(findDeleteVarBtnPlaceholder().exists()).toBe(true);
+  });
+
+  it('renders help text with provided link', () => {
+    expect(findHelpText().exists()).toBe(true);
+    expect(findHelpLink().attributes('href')).toBe(
+      '/help/ci/variables/index#add-a-cicd-variable-to-a-project',
+    );
+  });
+
+  it('passes variables in correct format', async () => {
+    jest.spyOn(store, 'dispatch');
+
+    await setCiVariableKey();
+
+    await findCiVariableValue().setValue('new value');
+
+    await findTriggerBtn().trigger('click');
+
+    expect(store.dispatch).toHaveBeenCalledWith('triggerManualJob', [
+      {
+        key: 'new key',
+        secret_value: 'new value',
+      },
+    ]);
   });
 });