Skip to content

G
gitlab-ce
Commit 77010754 authored by Payton Burdette's avatar Payton Burdette Committed by Frédéric Caplette
Browse files
Options

Refactor manual variables form 

Use one set of inputs for the variables
and change the UI a bit. Also removes technical
debt in the codebase by using best practices.

Changelog: changed
parent 7e240aed
Hide whitespace changes
Inline Side-by-side
Showing with 235 additions and 186 deletions
app/assets/javascripts/jobs/components/manual_variables_form.vue
View file @ 77010754
<script> <script>
import { GlButton, GlLink, GlSprintf } from '@gitlab/ui'; import {
GlFormInputGroup,
GlInputGroupText,
GlFormInput,
GlButton,
GlLink,
GlSprintf,
} from '@gitlab/ui';
import { uniqueId } from 'lodash'; import { uniqueId } from 'lodash';
import { mapActions } from 'vuex'; import { mapActions } from 'vuex';
import { helpPagePath } from '~/helpers/help_page_helper'; import { helpPagePath } from '~/helpers/help_page_helper';
...@@ -8,6 +15,9 @@ import { s__ } from '~/locale'; ...@@ -8,6 +15,9 @@ import { s__ } from '~/locale';
export default { export default {
name: 'ManualVariablesForm', name: 'ManualVariablesForm',
components: { components: {
GlFormInputGroup,
GlInputGroupText,
GlFormInput,
GlButton, GlButton,
GlLink, GlLink,
GlSprintf, GlSprintf,
...@@ -32,6 +42,9 @@ export default { ...@@ -32,6 +42,9 @@ export default {
value: 'value', value: 'value',
}, },
i18n: { i18n: {
header: s__('CiVariables|Variables'),
keyLabel: s__('CiVariables|Key'),
valueLabel: s__('CiVariables|Value'),
keyPlaceholder: s__('CiVariables|Input variable key'), keyPlaceholder: s__('CiVariables|Input variable key'),
valuePlaceholder: s__('CiVariables|Input variable value'), valuePlaceholder: s__('CiVariables|Input variable value'),
formHelpText: s__( formHelpText: s__(
...@@ -40,9 +53,13 @@ export default { ...@@ -40,9 +53,13 @@ export default {
}, },
data() { data() {
return { return {
variables: [], variables: [
key: '', {
secretValue: '', key: '',
secretValue: '',
id: uniqueId(),
},
],
triggerBtnDisabled: false, triggerBtnDisabled: false,
}; };
}, },
...@@ -50,40 +67,32 @@ export default { ...@@ -50,40 +67,32 @@ export default {
variableSettings() { variableSettings() {
return helpPagePath('ci/variables/index', { anchor: 'add-a-cicd-variable-to-a-project' }); return helpPagePath('ci/variables/index', { anchor: 'add-a-cicd-variable-to-a-project' });
}, },
}, preparedVariables() {
watch: { // we need to ensure no empty variables are passed to the API
key(newVal) { // and secretValue should be snake_case when passed to the API
this.handleValueChange(newVal, this.$options.inputTypes.key); return this.variables
}, .filter((variable) => variable.key !== '')
secretValue(newVal) { .map(({ key, secretValue }) => ({ key, secret_value: secretValue }));
this.handleValueChange(newVal, this.$options.inputTypes.value);
}, },
}, },
methods: { methods: {
...mapActions(['triggerManualJob']), ...mapActions(['triggerManualJob']),
handleValueChange(newValue, type) { canRemove(index) {
if (newValue !== '') { return index < this.variables.length - 1;
this.createNewVariable(type);
this.resetForm();
}
}, },
createNewVariable(type) { addEmptyVariable() {
const newVariable = { const lastVar = this.variables[this.variables.length - 1];
key: this.key,
secret_value: this.secretValue,
id: uniqueId(),
};
this.variables.push(newVariable); if (lastVar.key === '') {
return;
}
return this.$nextTick().then(() => { this.variables.push({
this.$refs[`${this.$options.inputTypes[type]}-${newVariable.id}`][0].focus(); key: '',
secret_value: '',
id: uniqueId(),
}); });
}, },
resetForm() {
this.key = '';
this.secretValue = '';
},
deleteVariable(id) { deleteVariable(id) {
this.variables.splice( this.variables.splice(
this.variables.findIndex((el) => el.id === id), this.variables.findIndex((el) => el.id === id),
...@@ -93,112 +102,92 @@ export default { ...@@ -93,112 +102,92 @@ export default {
trigger() { trigger() {
this.triggerBtnDisabled = true; this.triggerBtnDisabled = true;
this.triggerManualJob(this.variables); this.triggerManualJob(this.preparedVariables);
}, },
}, },
}; };
</script> </script>
<template> <template>
<div class="col-12" data-testid="manual-vars-form"> <div class="row gl-justify-content-center">
<label>{{ s__('CiVariables|Variables') }}</label> <div class="col-10" data-testid="manual-vars-form">
<label>{{ $options.i18n.header }}</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 <div
v-for="variable in variables" v-for="(variable, index) in variables"
:key="variable.id" :key="variable.id"
class="gl-responsive-table-row" class="gl-display-flex gl-align-items-center gl-mb-4"
data-testid="ci-variable-row" data-testid="ci-variable-row"
> >
<div class="table-section section-50"> <gl-form-input-group class="gl-mr-4 gl-flex-grow-1">
<div class="table-mobile-header" role="rowheader">{{ s__('Pipeline|Key') }}</div> <template #prepend>
<div class="table-mobile-content gl-mr-3"> <gl-input-group-text>
<input {{ $options.i18n.keyLabel }}
:ref="`${$options.inputTypes.key}-${variable.id}`" </gl-input-group-text>
v-model="variable.key" </template>
:placeholder="$options.i18n.keyPlaceholder" <gl-form-input
class="ci-variable-body-item form-control" :ref="`${$options.inputTypes.key}-${variable.id}`"
data-testid="ci-variable-key" v-model="variable.key"
/> :placeholder="$options.i18n.keyPlaceholder"
</div> data-testid="ci-variable-key"
</div> @change="addEmptyVariable"
/>
</gl-form-input-group>
<div class="table-section section-50"> <gl-form-input-group class="gl-flex-grow-2">
<div class="table-mobile-header" role="rowheader">{{ s__('Pipeline|Value') }}</div> <template #prepend>
<div class="table-mobile-content gl-mr-3"> <gl-input-group-text>
<input {{ $options.i18n.valueLabel }}
:ref="`${$options.inputTypes.value}-${variable.id}`" </gl-input-group-text>
v-model="variable.secret_value" </template>
:placeholder="$options.i18n.valuePlaceholder" <gl-form-input
class="ci-variable-body-item form-control" :ref="`${$options.inputTypes.value}-${variable.id}`"
data-testid="ci-variable-value" v-model="variable.secretValue"
/> :placeholder="$options.i18n.valuePlaceholder"
</div> data-testid="ci-variable-value"
</div> />
</gl-form-input-group>
<div class="table-section section-10"> <!-- delete variable button placeholder to not break flex layout -->
<div class="table-mobile-header" role="rowheader"></div> <div
<div class="table-mobile-content justify-content-end"> v-if="!canRemove(index)"
<gl-button class="gl-w-7 gl-mr-3"
category="tertiary" data-testid="delete-variable-btn-placeholder"
icon="clear" ></div>
:aria-label="__('Delete variable')"
data-testid="delete-variable-btn" <gl-button
@click="deleteVariable(variable.id)" v-if="canRemove(index)"
/> class="gl-flex-grow-0 gl-flex-basis-0"
</div> category="tertiary"
</div> variant="danger"
icon="clear"
:aria-label="__('Delete variable')"
data-testid="delete-variable-btn"
@click="deleteVariable(variable.id)"
/>
</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="gl-text-center gl-mt-5">
<div class="table-mobile-header" role="rowheader">{{ s__('Pipeline|Value') }}</div> <gl-sprintf :message="$options.i18n.formHelpText">
<div class="table-mobile-content gl-mr-3"> <template #link="{ content }">
<input <gl-link :href="variableSettings" target="_blank">
ref="inputSecretValue" {{ content }}
v-model="secretValue" </gl-link>
class="ci-variable-body-item form-control" </template>
:placeholder="$options.i18n.valuePlaceholder" </gl-sprintf>
/> </div>
</div> <div class="gl-display-flex gl-justify-content-center gl-mt-5">
</div> <gl-button
class="gl-mt-5"
variant="info"
category="primary"
:aria-label="__('Trigger manual job')"
:disabled="triggerBtnDisabled"
data-testid="trigger-manual-job-btn"
@click="trigger"
>
{{ action.button_title }}
</gl-button>
</div> </div>
</div>
<div class="gl-text-center gl-mt-3">
<gl-sprintf :message="$options.i18n.formHelpText">
<template #link="{ content }">
<gl-link :href="variableSettings" target="_blank">
{{ content }}
</gl-link>
</template>
</gl-sprintf>
</div>
<div class="d-flex justify-content-center">
<gl-button
variant="info"
category="primary"
:aria-label="__('Trigger manual job')"
:disabled="triggerBtnDisabled"
data-testid="trigger-manual-job-btn"
@click="trigger"
>
{{ action.button_title }}
</gl-button>
</div> </div>
</div> </div>
</template> </template>
locale/gitlab.pot
View file @ 77010754
...@@ -25525,9 +25525,6 @@ msgstr "" ...@@ -25525,9 +25525,6 @@ msgstr ""
msgid "Pipeline|In progress" msgid "Pipeline|In progress"
msgstr "" msgstr ""
msgid "Pipeline|Key"
msgstr ""
msgid "Pipeline|Manual" msgid "Pipeline|Manual"
msgstr "" msgstr ""
...@@ -25618,9 +25615,6 @@ msgstr "" ...@@ -25618,9 +25615,6 @@ msgstr ""
msgid "Pipeline|Triggerer" msgid "Pipeline|Triggerer"
msgstr "" msgstr ""
msgid "Pipeline|Value"
msgstr ""
msgid "Pipeline|Variables" msgid "Pipeline|Variables"
msgstr "" msgstr ""
......
spec/features/projects/jobs/user_triggers_manual_job_with_variables_spec.rb 0 → 100644
View file @ 77010754
# 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
View file @ 77010754
import { GlSprintf, GlLink } from '@gitlab/ui'; import { GlSprintf, GlLink } from '@gitlab/ui';
import { createLocalVue, mount, shallowMount } from '@vue/test-utils'; import { createLocalVue, mount } from '@vue/test-utils';
import Vue from 'vue'; import Vue, { nextTick } from 'vue';
import Vuex from 'vuex'; import Vuex from 'vuex';
import { extendedWrapper } from 'helpers/vue_test_utils_helper'; 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(); const localVue = createLocalVue();
...@@ -21,7 +21,7 @@ describe('Manual Variables Form', () => { ...@@ -21,7 +21,7 @@ describe('Manual Variables Form', () => {
}, },
}; };
const createComponent = ({ props = {}, mountFn = shallowMount } = {}) => { const createComponent = (props = {}) => {
store = new Vuex.Store({ store = new Vuex.Store({
actions: { actions: {
triggerManualJob: jest.fn(), triggerManualJob: jest.fn(),
...@@ -29,7 +29,7 @@ describe('Manual Variables Form', () => { ...@@ -29,7 +29,7 @@ describe('Manual Variables Form', () => {
}); });
wrapper = extendedWrapper( wrapper = extendedWrapper(
mountFn(localVue.extend(Form), { mount(localVue.extend(ManualVariablesForm), {
propsData: { ...requiredProps, ...props }, propsData: { ...requiredProps, ...props },
localVue, localVue,
store, store,
...@@ -40,88 +40,120 @@ describe('Manual Variables Form', () => { ...@@ -40,88 +40,120 @@ describe('Manual Variables Form', () => {
); );
}; };
const findInputKey = () => wrapper.findComponent({ ref: 'inputKey' });
const findInputValue = () => wrapper.findComponent({ ref: 'inputSecretValue' });
const findHelpText = () => wrapper.findComponent(GlSprintf); const findHelpText = () => wrapper.findComponent(GlSprintf);
const findHelpLink = () => wrapper.findComponent(GlLink); const findHelpLink = () => wrapper.findComponent(GlLink);
const findTriggerBtn = () => wrapper.findByTestId('trigger-manual-job-btn'); const findTriggerBtn = () => wrapper.findByTestId('trigger-manual-job-btn');
const findDeleteVarBtn = () => wrapper.findByTestId('delete-variable-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 findCiVariableKey = () => wrapper.findByTestId('ci-variable-key');
const findAllCiVariableKeys = () => wrapper.findAllByTestId('ci-variable-key');
const findCiVariableValue = () => wrapper.findByTestId('ci-variable-value'); const findCiVariableValue = () => wrapper.findByTestId('ci-variable-value');
const findAllVariables = () => wrapper.findAllByTestId('ci-variable-row'); const findAllVariables = () => wrapper.findAllByTestId('ci-variable-row');
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();
};
beforeEach(() => {
createComponent();
});
afterEach(() => { afterEach(() => {
wrapper.destroy(); wrapper.destroy();
}); });
describe('shallowMount', () => { it('creates a new variable when user enters a new key value', async () => {
beforeEach(() => { expect(findAllVariables()).toHaveLength(1);
createComponent();
});
it('renders empty form with correct placeholders', () => { await setCiVariableKey();
expect(findInputKey().attributes('placeholder')).toBe('Input variable key');
expect(findInputValue().attributes('placeholder')).toBe('Input variable value');
});
it('renders help text with provided link', () => { expect(findAllVariables()).toHaveLength(2);
expect(findHelpText().exists()).toBe(true); });
expect(findHelpLink().attributes('href')).toBe(
'/help/ci/variables/index#add-a-cicd-variable-to-a-project',
);
});
describe('when adding a new variable', () => { it('does not create extra empty variables', async () => {
it('creates a new variable when user types a new key and resets the form', async () => { expect(findAllVariables()).toHaveLength(1);
await findInputKey().setValue('new key');
expect(findAllVariables()).toHaveLength(1); await setCiVariableKey();
expect(findCiVariableKey().element.value).toBe('new key');
expect(findInputKey().attributes('value')).toBe(undefined);
});
it('creates a new variable when user types a new value and resets the form', async () => { expect(findAllVariables()).toHaveLength(2);
await findInputValue().setValue('new value');
expect(findAllVariables()).toHaveLength(1); await setCiVariableKey();
expect(findCiVariableValue().element.value).toBe('new value');
expect(findInputValue().attributes('value')).toBe(undefined); expect(findAllVariables()).toHaveLength(2);
});
});
}); });
describe('mount', () => { it('removes the correct variable row', async () => {
beforeEach(() => { const variableKeyNameOne = 'key-one';
createComponent({ mountFn: mount }); const variableKeyNameThree = 'key-three';
});
describe('when deleting a variable', () => { await setCiVariableKeyByPosition(0, variableKeyNameOne);
it('removes the variable row', async () => {
await wrapper.setData({
variables: [
{
key: 'new key',
secret_value: 'value',
id: '1',
},
],
});
findDeleteVarBtn().trigger('click'); await setCiVariableKeyByPosition(1, 'key-two');
await wrapper.vm.$nextTick(); await setCiVariableKeyByPosition(2, variableKeyNameThree);
expect(findAllVariables()).toHaveLength(0); expect(findAllVariables()).toHaveLength(4);
});
});
it('trigger button is disabled after trigger action', async () => { await findAllDeleteVarBtns().at(1).trigger('click');
expect(findTriggerBtn().props('disabled')).toBe(false);
await findTriggerBtn().trigger('click'); expect(findAllVariables()).toHaveLength(3);
expect(findTriggerBtn().props('disabled')).toBe(true); 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 () => {
expect(findTriggerBtn().props('disabled')).toBe(false);
await findTriggerBtn().trigger('click');
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',
},
]);
}); });
}); });
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7