From 60bfb226ad2dc2193d858ec49a9163429dd9f752 Mon Sep 17 00:00:00 2001 From: mgandres Date: Tue, 23 Aug 2022 03:07:56 +0800 Subject: [PATCH 1/3] Migrate Run Pipeline form to GraphQL and fetch predefined variables This migrates the form from axios to GraphQL. The form submission still uses a client resolver, but the query to fetch the CiConfig variables now uses the new GQL endpoint. This uses the `run_pipeline_graphql` feature flag. --- .../components/pipeline_new_form.vue | 253 ++++++++---------- .../create_pipeline.mutation.graphql | 9 + .../queries/ci_config_variables.graphql | 10 + .../pipeline_new/graphql/resolvers.js | 29 ++ app/assets/javascripts/pipeline_new/index.js | 14 +- app/views/projects/pipelines/new.html.haml | 1 + .../components/pipeline_new_form_spec.js | 229 ++++++++-------- spec/frontend/pipeline_new/mock_data.js | 55 ++++ 8 files changed, 344 insertions(+), 256 deletions(-) create mode 100644 app/assets/javascripts/pipeline_new/graphql/mutations/create_pipeline.mutation.graphql create mode 100644 app/assets/javascripts/pipeline_new/graphql/queries/ci_config_variables.graphql create mode 100644 app/assets/javascripts/pipeline_new/graphql/resolvers.js diff --git a/app/assets/javascripts/pipeline_new/components/pipeline_new_form.vue b/app/assets/javascripts/pipeline_new/components/pipeline_new_form.vue index cd7cb7f839333e..e274ecef8585a2 100644 --- a/app/assets/javascripts/pipeline_new/components/pipeline_new_form.vue +++ b/app/assets/javascripts/pipeline_new/components/pipeline_new_form.vue @@ -17,17 +17,11 @@ import { import * as Sentry from '@sentry/browser'; import { uniqueId } from 'lodash'; import Vue from 'vue'; -import axios from '~/lib/utils/axios_utils'; -import { backOff } from '~/lib/utils/common_utils'; -import httpStatusCodes from '~/lib/utils/http_status'; import { redirectTo } from '~/lib/utils/url_utility'; import { s__, __, n__ } from '~/locale'; -import { - VARIABLE_TYPE, - FILE_TYPE, - CONFIG_VARIABLES_TIMEOUT, - CC_VALIDATION_REQUIRED_ERROR, -} from '../constants'; +import { VARIABLE_TYPE, FILE_TYPE, CC_VALIDATION_REQUIRED_ERROR } from '../constants'; +import createPipelineMutation from '../graphql/mutations/create_pipeline.mutation.graphql'; +import ciConfigVariablesQuery from '../graphql/queries/ci_config_variables.graphql'; import filterVariables from '../utils/filter_variables'; import RefsDropdown from './refs_dropdown.vue'; @@ -76,10 +70,6 @@ export default { type: String, required: true, }, - configVariablesPath: { - type: String, - required: true, - }, defaultBranch: { type: String, required: true, @@ -97,6 +87,10 @@ export default { required: false, default: () => ({}), }, + projectPath: { + type: String, + required: true, + }, refParam: { type: String, required: false, @@ -116,19 +110,78 @@ export default { return { refValue: { shortName: this.refParam, + // this is needed until we add support for ref type in url query strings + // ensure default branch is called with full ref on load + // https://gitlab.com/gitlab-org/gitlab/-/issues/287815 + fullName: this.refParam === this.defaultBranch ? `refs/heads/${this.refParam}` : undefined, }, form: {}, errorTitle: null, error: null, + predefinedVariables: {}, warnings: [], totalWarnings: 0, isWarningDismissed: false, - isLoading: false, submitted: false, ccAlertDismissed: false, }; }, + apollo: { + ciConfigVariables: { + query: ciConfigVariablesQuery, + // Skip when variables already cached in `form` + skip() { + return Object.keys(this.form).includes(this.refFullName); + }, + variables() { + return { + fullPath: this.projectPath, + sha: this.refQueryParam, + }; + }, + update(data) { + return data?.project?.ciConfigVariables || []; + }, + result({ data }) { + const predefinedVars = data?.project?.ciConfigVariables || []; + const params = {}; + const descriptions = {}; + + predefinedVars.forEach(({ description, key, value }) => { + if (description) { + const valueOptions = value.includes('[') ? JSON.parse(value) : [value]; + [params[key]] = valueOptions; + descriptions[key] = description; + this.predefinedVariables[key] = valueOptions; + } + }); + + Vue.set(this.form, this.refFullName, { descriptions, variables: [] }); + + // Add default variables from yml + this.setVariableParams(this.refFullName, VARIABLE_TYPE, params); + + // Add/update variables, e.g. from query string + if (this.variableParams) { + this.setVariableParams(this.refFullName, VARIABLE_TYPE, this.variableParams); + } + + if (this.fileParams) { + this.setVariableParams(this.refFullName, FILE_TYPE, this.fileParams); + } + + // Adds empty var at the end of the form + this.addEmptyVariable(this.refFullName); + }, + error(error) { + Sentry.captureException(error); + }, + }, + }, computed: { + isLoading() { + return this.$apollo.queries.ciConfigVariables.loading; + }, overMaxWarningsLimit() { return this.totalWarnings > this.maxWarnings; }, @@ -147,6 +200,9 @@ export default { refFullName() { return this.refValue.fullName; }, + refQueryParam() { + return this.refFullName || this.refShortName; + }, variables() { return this.form[this.refFullName]?.variables ?? []; }, @@ -157,21 +213,6 @@ export default { return this.error === CC_VALIDATION_REQUIRED_ERROR && !this.ccAlertDismissed; }, }, - watch: { - refValue() { - this.loadConfigVariablesForm(); - }, - }, - created() { - // this is needed until we add support for ref type in url query strings - // ensure default branch is called with full ref on load - // https://gitlab.com/gitlab-org/gitlab/-/issues/287815 - if (this.refValue.shortName === this.defaultBranch) { - this.refValue.fullName = `refs/heads/${this.refValue.shortName}`; - } - - this.loadConfigVariablesForm(); - }, methods: { addEmptyVariable(refValue) { const { variables } = this.form[refValue]; @@ -204,132 +245,57 @@ export default { }); } }, - setVariableType(key, type) { + setVariableAttribute(key, attribute, value) { const { variables } = this.form[this.refFullName]; const variable = variables.find((v) => v.key === key); - variable.variable_type = type; + variable[attribute] = value; }, setVariableParams(refValue, type, paramsObj) { Object.entries(paramsObj).forEach(([key, value]) => { this.setVariable(refValue, type, key, value); }); }, + shouldShowValuesDropdown(key) { + return this.predefinedVariables[key] && this.predefinedVariables[key].length > 1; + }, removeVariable(index) { this.variables.splice(index, 1); }, canRemove(index) { return index < this.variables.length - 1; }, - loadConfigVariablesForm() { - // Skip when variables already cached in `form` - if (this.form[this.refFullName]) { - return; - } - - this.fetchConfigVariables(this.refFullName || this.refShortName) - .then(({ descriptions, params }) => { - Vue.set(this.form, this.refFullName, { - variables: [], - descriptions, - }); - - // Add default variables from yml - this.setVariableParams(this.refFullName, VARIABLE_TYPE, params); - }) - .catch(() => { - Vue.set(this.form, this.refFullName, { - variables: [], - descriptions: {}, - }); - }) - .finally(() => { - // Add/update variables, e.g. from query string - if (this.variableParams) { - this.setVariableParams(this.refFullName, VARIABLE_TYPE, this.variableParams); - } - if (this.fileParams) { - this.setVariableParams(this.refFullName, FILE_TYPE, this.fileParams); - } - - // Adds empty var at the end of the form - this.addEmptyVariable(this.refFullName); - }); - }, - fetchConfigVariables(refValue) { - this.isLoading = true; - - return backOff((next, stop) => { - axios - .get(this.configVariablesPath, { - params: { - sha: refValue, - }, - }) - .then(({ data, status }) => { - if (status === httpStatusCodes.NO_CONTENT) { - next(); - } else { - this.isLoading = false; - stop(data); - } - }) - .catch((error) => { - stop(error); - }); - }, CONFIG_VARIABLES_TIMEOUT) - .then((data) => { - const params = {}; - const descriptions = {}; - - Object.entries(data).forEach(([key, { value, description }]) => { - if (description) { - params[key] = value; - descriptions[key] = description; - } - }); - - return { params, descriptions }; - }) - .catch((error) => { - this.isLoading = false; - - Sentry.captureException(error); - - return { params: {}, descriptions: {} }; - }); - }, - createPipeline() { + async createPipeline() { this.submitted = true; this.ccAlertDismissed = false; - return axios - .post(this.pipelinesPath, { + const { data } = await this.$apollo.mutate({ + mutation: createPipelineMutation, + variables: { + endpoint: this.pipelinesPath, // send shortName as fall back for query params // https://gitlab.com/gitlab-org/gitlab/-/issues/287815 - ref: this.refValue.fullName || this.refShortName, - variables_attributes: filterVariables(this.variables), - }) - .then(({ data }) => { - redirectTo(`${this.pipelinesPath}/${data.id}`); - }) - .catch((err) => { - // always re-enable submit button - this.submitted = false; + ref: this.refQueryParam, + variablesAttributes: filterVariables(this.variables), + }, + }); - const { - errors = [], - warnings = [], - total_warnings: totalWarnings = 0, - } = err.response.data; - const [error] = errors; + const { id, errors, totalWarnings, warnings } = data.createPipeline; - this.reportError({ - title: i18n.submitErrorTitle, - error, - warnings, - totalWarnings, - }); - }); + if (id) { + redirectTo(`${this.pipelinesPath}/${id}`); + return; + } + + // always re-enable submit button + this.submitted = false; + const [error] = errors; + + this.reportError({ + title: i18n.submitErrorTitle, + error, + warnings, + totalWarnings, + }); }, onRefsLoadingError(error) { this.reportError({ title: i18n.refsLoadingErrorTitle }); @@ -416,7 +382,7 @@ export default { {{ $options.typeOptions[type] }} @@ -429,7 +395,24 @@ export default { data-qa-selector="ci_variable_key_field" @change="addEmptyVariable(refFullName)" /> + + + {{ value }} + + { + return axios + .post(endpoint, { ref, variables_attributes: variablesAttributes }) + .then((response) => { + const { id } = response.data; + return { + id, + errors: [], + totalWarnings: 0, + warnings: [], + }; + }) + .catch((err) => { + const { errors = [], totalWarnings = 0, warnings = [] } = err.response.data; + + return { + id: null, + errors, + totalWarnings, + warnings, + }; + }); + }, + }, +}; diff --git a/app/assets/javascripts/pipeline_new/index.js b/app/assets/javascripts/pipeline_new/index.js index e3f363f4ada4e4..60b4c93d1d50da 100644 --- a/app/assets/javascripts/pipeline_new/index.js +++ b/app/assets/javascripts/pipeline_new/index.js @@ -1,6 +1,9 @@ import Vue from 'vue'; +import VueApollo from 'vue-apollo'; +import createDefaultClient from '~/lib/graphql'; import LegacyPipelineNewForm from './components/legacy_pipeline_new_form.vue'; import PipelineNewForm from './components/pipeline_new_form.vue'; +import { resolvers } from './graphql/resolvers'; const mountLegacyPipelineNewForm = (el) => { const { @@ -51,12 +54,12 @@ const mountPipelineNewForm = (el) => { projectRefsEndpoint, // props - configVariablesPath, defaultBranch, fileParam, maxWarnings, pipelinesPath, projectId, + projectPath, refParam, settingsLink, varParam, @@ -65,22 +68,27 @@ const mountPipelineNewForm = (el) => { const variableParams = JSON.parse(varParam); const fileParams = JSON.parse(fileParam); - // TODO: add apolloProvider + Vue.use(VueApollo); + + const apolloProvider = new VueApollo({ + defaultClient: createDefaultClient(resolvers), + }); return new Vue({ el, + apolloProvider, provide: { projectRefsEndpoint, }, render(createElement) { return createElement(PipelineNewForm, { props: { - configVariablesPath, defaultBranch, fileParams, maxWarnings: Number(maxWarnings), pipelinesPath, projectId, + projectPath, refParam, settingsLink, variableParams, diff --git a/app/views/projects/pipelines/new.html.haml b/app/views/projects/pipelines/new.html.haml index a4144f8ab0dba7..d2b2a58fcf8af1 100644 --- a/app/views/projects/pipelines/new.html.haml +++ b/app/views/projects/pipelines/new.html.haml @@ -12,6 +12,7 @@ ref_param: params[:ref] || @project.default_branch, var_param: params[:var].to_json, file_param: params[:file_var].to_json, + project_path: @project.full_path, project_refs_endpoint: refs_project_path(@project, sort: 'updated_desc'), settings_link: project_settings_ci_cd_path(@project), max_warnings: ::Gitlab::Ci::Warnings::MAX_LIMIT } } diff --git a/spec/frontend/pipeline_new/components/pipeline_new_form_spec.js b/spec/frontend/pipeline_new/components/pipeline_new_form_spec.js index 5ce29bd6c5dc77..afe3ad197247f3 100644 --- a/spec/frontend/pipeline_new/components/pipeline_new_form_spec.js +++ b/spec/frontend/pipeline_new/components/pipeline_new_form_spec.js @@ -1,72 +1,101 @@ -import { GlForm, GlSprintf, GlLoadingIcon } from '@gitlab/ui'; -import { mount, shallowMount } from '@vue/test-utils'; +import Vue, { nextTick } from 'vue'; +import VueApollo from 'vue-apollo'; +import { GlForm, GlDropdownItem, GlSprintf, GlLoadingIcon } from '@gitlab/ui'; import MockAdapter from 'axios-mock-adapter'; -import { nextTick } from 'vue'; import CreditCardValidationRequiredAlert from 'ee_component/billings/components/cc_validation_required_alert.vue'; +import createMockApollo from 'helpers/mock_apollo_helper'; +import { shallowMountExtended, mountExtended } from 'helpers/vue_test_utils_helper'; import { TEST_HOST } from 'helpers/test_constants'; import waitForPromises from 'helpers/wait_for_promises'; import axios from '~/lib/utils/axios_utils'; import httpStatusCodes from '~/lib/utils/http_status'; import { redirectTo } from '~/lib/utils/url_utility'; import PipelineNewForm from '~/pipeline_new/components/pipeline_new_form.vue'; +import ciConfigVariablesQuery from '~/pipeline_new/graphql/queries/ci_config_variables.graphql'; +import { resolvers } from '~/pipeline_new/graphql/resolvers'; import RefsDropdown from '~/pipeline_new/components/refs_dropdown.vue'; import { + mockCreditCardValidationRequiredError, + mockCiConfigVariablesResponse, + mockCiConfigVariablesResponseWithoutDesc, + mockEmptyCiConfigVariablesResponse, + mockError, mockQueryParams, mockPostParams, mockProjectId, - mockError, mockRefs, - mockCreditCardValidationRequiredError, + mockYamlVariables, } from '../mock_data'; +Vue.use(VueApollo); + jest.mock('~/lib/utils/url_utility', () => ({ redirectTo: jest.fn(), })); const projectRefsEndpoint = '/root/project/refs'; const pipelinesPath = '/root/project/-/pipelines'; -const configVariablesPath = '/root/project/-/pipelines/config_variables'; +const projectPath = '/root/project/-/pipelines/config_variables'; const newPipelinePostResponse = { id: 1 }; const defaultBranch = 'main'; describe('Pipeline New Form', () => { let wrapper; let mock; + let mockApollo; + let mockCiConfigVariables; let dummySubmitEvent; const findForm = () => wrapper.findComponent(GlForm); const findRefsDropdown = () => wrapper.findComponent(RefsDropdown); - const findSubmitButton = () => wrapper.find('[data-testid="run_pipeline_button"]'); - const findVariableRows = () => wrapper.findAll('[data-testid="ci-variable-row"]'); - const findRemoveIcons = () => wrapper.findAll('[data-testid="remove-ci-variable-row"]'); - const findDropdowns = () => wrapper.findAll('[data-testid="pipeline-form-ci-variable-type"]'); - const findKeyInputs = () => wrapper.findAll('[data-testid="pipeline-form-ci-variable-key"]'); - const findValueInputs = () => wrapper.findAll('[data-testid="pipeline-form-ci-variable-value"]'); - const findErrorAlert = () => wrapper.find('[data-testid="run-pipeline-error-alert"]'); - const findWarningAlert = () => wrapper.find('[data-testid="run-pipeline-warning-alert"]'); + const findSubmitButton = () => wrapper.findByTestId('run_pipeline_button'); + const findVariableRows = () => wrapper.findAllByTestId('ci-variable-row'); + const findRemoveIcons = () => wrapper.findAllByTestId('remove-ci-variable-row'); + const findVariableTypes = () => wrapper.findAllByTestId('pipeline-form-ci-variable-type'); + const findKeyInputs = () => wrapper.findAllByTestId('pipeline-form-ci-variable-key'); + const findValueInputs = () => wrapper.findAllByTestId('pipeline-form-ci-variable-value'); + const findValueDropdowns = () => + wrapper.findAllByTestId('pipeline-form-ci-variable-value-dropdown'); + const findValueDropdownItems = (dropdown) => dropdown.findAllComponents(GlDropdownItem); + const findErrorAlert = () => wrapper.findByTestId('run-pipeline-error-alert'); + const findWarningAlert = () => wrapper.findByTestId('run-pipeline-warning-alert'); const findWarningAlertSummary = () => findWarningAlert().findComponent(GlSprintf); - const findWarnings = () => wrapper.findAll('[data-testid="run-pipeline-warning"]'); + const findWarnings = () => wrapper.findAllByTestId('run-pipeline-warning'); const findLoadingIcon = () => wrapper.findComponent(GlLoadingIcon); const findCCAlert = () => wrapper.findComponent(CreditCardValidationRequiredAlert); const getFormPostParams = () => JSON.parse(mock.history.post[0].data); - const selectBranch = (branch) => { + const selectBranch = async (branch) => { // Select a branch in the dropdown findRefsDropdown().vm.$emit('input', { shortName: branch, fullName: `refs/heads/${branch}`, }); + + await waitForPromises(); }; - const createComponent = (props = {}, method = shallowMount) => { + const changeKeyInputValue = async (keyInputIndex, value) => { + const input = findKeyInputs().at(keyInputIndex); + input.element.value = value; + input.trigger('change'); + + await nextTick(); + }; + + const createComponentWithApollo = ({ method = shallowMountExtended, props = {} } = {}) => { + const handlers = [[ciConfigVariablesQuery, mockCiConfigVariables]]; + mockApollo = createMockApollo(handlers, resolvers); + wrapper = method(PipelineNewForm, { + apolloProvider: mockApollo, provide: { projectRefsEndpoint, }, propsData: { projectId: mockProjectId, pipelinesPath, - configVariablesPath, + projectPath, defaultBranch, refParam: defaultBranch, settingsLink: '', @@ -78,7 +107,7 @@ describe('Pipeline New Form', () => { beforeEach(() => { mock = new MockAdapter(axios); - mock.onGet(configVariablesPath).reply(httpStatusCodes.OK, {}); + mockCiConfigVariables = jest.fn(); mock.onGet(projectRefsEndpoint).reply(httpStatusCodes.OK, mockRefs); dummySubmitEvent = { @@ -87,24 +116,19 @@ describe('Pipeline New Form', () => { }); afterEach(() => { - wrapper.destroy(); - wrapper = null; - mock.restore(); }); describe('Form', () => { beforeEach(async () => { - createComponent(mockQueryParams, mount); - - mock.onPost(pipelinesPath).reply(httpStatusCodes.OK, newPipelinePostResponse); - + mockCiConfigVariables.mockResolvedValue(mockEmptyCiConfigVariablesResponse); + createComponentWithApollo({ props: mockQueryParams, method: mountExtended }); await waitForPromises(); }); it('displays the correct values for the provided query params', async () => { - expect(findDropdowns().at(0).props('text')).toBe('Variable'); - expect(findDropdowns().at(1).props('text')).toBe('File'); + expect(findVariableTypes().at(0).props('text')).toBe('Variable'); + expect(findVariableTypes().at(1).props('text')).toBe('File'); expect(findRefsDropdown().props('value')).toEqual({ shortName: 'tag-1' }); expect(findVariableRows()).toHaveLength(3); }); @@ -117,7 +141,7 @@ describe('Pipeline New Form', () => { it('displays an empty variable for the user to fill out', async () => { expect(findKeyInputs().at(2).element.value).toBe(''); expect(findValueInputs().at(2).element.value).toBe(''); - expect(findDropdowns().at(2).props('text')).toBe('Variable'); + expect(findVariableTypes().at(2).props('text')).toBe('Variable'); }); it('does not display remove icon for last row', () => { @@ -147,13 +171,12 @@ describe('Pipeline New Form', () => { describe('Pipeline creation', () => { beforeEach(async () => { + mockCiConfigVariables.mockResolvedValue(mockEmptyCiConfigVariablesResponse); mock.onPost(pipelinesPath).reply(httpStatusCodes.OK, newPipelinePostResponse); - - await waitForPromises(); }); it('does not submit the native HTML form', async () => { - createComponent(); + createComponentWithApollo(); findForm().vm.$emit('submit', dummySubmitEvent); @@ -161,7 +184,7 @@ describe('Pipeline New Form', () => { }); it('disables the submit button immediately after submitting', async () => { - createComponent(); + createComponentWithApollo(); expect(findSubmitButton().props('disabled')).toBe(false); @@ -172,7 +195,7 @@ describe('Pipeline New Form', () => { }); it('creates pipeline with full ref and variables', async () => { - createComponent(); + createComponentWithApollo(); findForm().vm.$emit('submit', dummySubmitEvent); await waitForPromises(); @@ -182,7 +205,7 @@ describe('Pipeline New Form', () => { }); it('creates a pipeline with short ref and variables from the query params', async () => { - createComponent(mockQueryParams); + createComponentWithApollo({ props: mockQueryParams }); await waitForPromises(); @@ -197,41 +220,25 @@ describe('Pipeline New Form', () => { describe('When the ref has been changed', () => { beforeEach(async () => { - createComponent({}, mount); + mockCiConfigVariables.mockResolvedValue(mockEmptyCiConfigVariablesResponse); + createComponentWithApollo({ method: mountExtended }); await waitForPromises(); }); - it('variables persist between ref changes', async () => { - selectBranch('main'); - - await waitForPromises(); - - const mainInput = findKeyInputs().at(0); - mainInput.element.value = 'build_var'; - mainInput.trigger('change'); - - await nextTick(); - - selectBranch('branch-1'); - - await waitForPromises(); - - const branchOneInput = findKeyInputs().at(0); - branchOneInput.element.value = 'deploy_var'; - branchOneInput.trigger('change'); - await nextTick(); + it('variables persist between ref changes', async () => { + await selectBranch('main'); + await changeKeyInputValue(0, 'build_var'); - selectBranch('main'); + await selectBranch('branch-1'); + await changeKeyInputValue(0, 'deploy_var'); - await waitForPromises(); + await selectBranch('main'); expect(findKeyInputs().at(0).element.value).toBe('build_var'); expect(findVariableRows().length).toBe(2); - selectBranch('branch-1'); - - await waitForPromises(); + await selectBranch('branch-1'); expect(findKeyInputs().at(0).element.value).toBe('deploy_var'); expect(findVariableRows().length).toBe(2); @@ -239,22 +246,9 @@ describe('Pipeline New Form', () => { }); describe('when yml defines a variable', () => { - const mockYmlKey = 'yml_var'; - const mockYmlValue = 'yml_var_val'; - const mockYmlMultiLineValue = `A value - with multiple - lines`; - const mockYmlDesc = 'A var from yml.'; - it('loading icon is shown when content is requested and hidden when received', async () => { - createComponent(mockQueryParams, mount); - - mock.onGet(configVariablesPath).reply(httpStatusCodes.OK, { - [mockYmlKey]: { - value: mockYmlValue, - description: mockYmlDesc, - }, - }); + mockCiConfigVariables.mockResolvedValue(mockEmptyCiConfigVariablesResponse); + createComponentWithApollo({ props: mockQueryParams, method: mountExtended }); expect(findLoadingIcon().exists()).toBe(true); @@ -263,51 +257,62 @@ describe('Pipeline New Form', () => { expect(findLoadingIcon().exists()).toBe(false); }); - it('multi-line strings are added to the value field without removing line breaks', async () => { - createComponent(mockQueryParams, mount); + describe('with different predefined values', () => { + beforeEach(async () => { + mockCiConfigVariables.mockResolvedValue(mockCiConfigVariablesResponse); + createComponentWithApollo({ method: mountExtended }); + await waitForPromises(); + }); + + it('multi-line strings are added to the value field without removing line breaks', () => { + expect(findValueInputs().at(1).element.value).toBe(mockYamlVariables[1].value); + }); + + it('multiple predefined values are rendered as a dropdown', () => { + const dropdown = findValueDropdowns().at(0); + const dropdownItems = findValueDropdownItems(dropdown); + const yamlValue = JSON.parse(mockYamlVariables[2].value); - mock.onGet(configVariablesPath).reply(httpStatusCodes.OK, { - [mockYmlKey]: { - value: mockYmlMultiLineValue, - description: mockYmlDesc, - }, + expect(dropdownItems.at(0).text()).toBe(yamlValue[0]); + expect(dropdownItems.at(1).text()).toBe(yamlValue[1]); + expect(dropdownItems.at(2).text()).toBe(yamlValue[2]); }); - await waitForPromises(); + it('variables with multiple predefined values sets the first option as the default', () => { + const dropdown = findValueDropdowns().at(0); + const yamlValue = JSON.parse(mockYamlVariables[2].value); - expect(findValueInputs().at(0).element.value).toBe(mockYmlMultiLineValue); + expect(dropdown.props('text')).toBe(yamlValue[0]); + }); }); describe('with description', () => { beforeEach(async () => { - createComponent(mockQueryParams, mount); - - mock.onGet(configVariablesPath).reply(httpStatusCodes.OK, { - [mockYmlKey]: { - value: mockYmlValue, - description: mockYmlDesc, - }, - }); - + mockCiConfigVariables.mockResolvedValue(mockCiConfigVariablesResponse); + createComponentWithApollo({ props: mockQueryParams, method: mountExtended }); await waitForPromises(); }); it('displays all the variables', async () => { - expect(findVariableRows()).toHaveLength(4); + expect(findVariableRows()).toHaveLength(6); }); it('displays a variable from yml', () => { - expect(findKeyInputs().at(0).element.value).toBe(mockYmlKey); - expect(findValueInputs().at(0).element.value).toBe(mockYmlValue); + expect(findKeyInputs().at(0).element.value).toBe(mockYamlVariables[0].key); + expect(findValueInputs().at(0).element.value).toBe(mockYamlVariables[0].value); }); it('displays a variable from provided query params', () => { - expect(findKeyInputs().at(1).element.value).toBe('test_var'); - expect(findValueInputs().at(1).element.value).toBe('test_var_val'); + expect(findKeyInputs().at(3).element.value).toBe( + Object.keys(mockQueryParams.variableParams)[0], + ); + expect(findValueInputs().at(3).element.value).toBe( + Object.values(mockQueryParams.fileParams)[0], + ); }); it('adds a description to the first variable from yml', () => { - expect(findVariableRows().at(0).text()).toContain(mockYmlDesc); + expect(findVariableRows().at(0).text()).toContain(mockYamlVariables[0].description); }); it('removes the description when a variable key changes', async () => { @@ -316,39 +321,27 @@ describe('Pipeline New Form', () => { await nextTick(); - expect(findVariableRows().at(0).text()).not.toContain(mockYmlDesc); + expect(findVariableRows().at(0).text()).not.toContain(mockYamlVariables[0].description); }); }); describe('without description', () => { beforeEach(async () => { - createComponent(mockQueryParams, mount); - - mock.onGet(configVariablesPath).reply(httpStatusCodes.OK, { - [mockYmlKey]: { - value: mockYmlValue, - description: null, - }, - yml_var2: { - value: 'yml_var2_val', - }, - yml_var3: { - description: '', - }, - }); - + mockCiConfigVariables.mockResolvedValue(mockCiConfigVariablesResponseWithoutDesc); + createComponentWithApollo({ method: mountExtended }); await waitForPromises(); }); - it('displays all the variables', async () => { - expect(findVariableRows()).toHaveLength(3); + it('displays variables with description only', async () => { + expect(findVariableRows()).toHaveLength(2); // extra empty variable is added at the end }); }); }); describe('Form errors and warnings', () => { beforeEach(() => { - createComponent(); + mockCiConfigVariables.mockResolvedValue(mockEmptyCiConfigVariablesResponse); + createComponentWithApollo(); }); describe('when the refs cannot be loaded', () => { diff --git a/spec/frontend/pipeline_new/mock_data.js b/spec/frontend/pipeline_new/mock_data.js index e99684ff417c37..362b352b61165e 100644 --- a/spec/frontend/pipeline_new/mock_data.js +++ b/spec/frontend/pipeline_new/mock_data.js @@ -65,3 +65,58 @@ export const mockVariables = [ }, { uniqueId: 'var-refs/heads/main4', variable_type: 'env_var', key: '', value: '' }, ]; + +export const mockYamlVariables = [ + { + description: 'This is a variable with a value.', + key: 'VAR_WITH_VALUE', + value: 'test_value', + }, + { + description: 'This is a variable with a multi-line value.', + key: 'VAR_WITH_MULTILINE', + value: `this is + a multiline + value`, + }, + { + description: 'This is a variable with predefined values.', + key: 'VAR_WITH_OPTIONS', + value: '["development", "staging", "production"]', + }, +]; + +export const mockYamlVariablesWithoutDesc = [ + { + description: 'This is a variable with a value.', + key: 'VAR_WITH_VALUE', + value: 'test_value', + }, + { + description: null, + key: 'VAR_WITH_MULTILINE', + value: `this is + a multiline + value`, + }, + { + description: null, + key: 'VAR_WITH_OPTIONS', + value: '["development", "staging", "production"]', + }, +]; + +export const mockCiConfigVariablesQueryResponse = (ciConfigVariables) => ({ + data: { + project: { + id: 1, + ciConfigVariables, + }, + }, +}); + +export const mockCiConfigVariablesResponse = mockCiConfigVariablesQueryResponse(mockYamlVariables); +export const mockEmptyCiConfigVariablesResponse = mockCiConfigVariablesQueryResponse([]); +export const mockCiConfigVariablesResponseWithoutDesc = mockCiConfigVariablesQueryResponse( + mockYamlVariablesWithoutDesc, +); -- GitLab From ba5bc0f1703b8ecc57830e201d1179785357c509 Mon Sep 17 00:00:00 2001 From: mgandres Date: Mon, 19 Sep 2022 17:15:12 +0800 Subject: [PATCH 2/3] Update MR with reviewer feedback This also removes the transformation on the returned GQL data, since the query now always returns an array of strings for the variable value. --- .../components/pipeline_new_form.vue | 11 ++++---- .../queries/ci_config_variables.graphql | 4 +-- .../components/pipeline_new_form_spec.js | 25 ++++++++++++++++--- spec/frontend/pipeline_new/mock_data.js | 22 ++++++++-------- 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/app/assets/javascripts/pipeline_new/components/pipeline_new_form.vue b/app/assets/javascripts/pipeline_new/components/pipeline_new_form.vue index e274ecef8585a2..dc08d5339ba270 100644 --- a/app/assets/javascripts/pipeline_new/components/pipeline_new_form.vue +++ b/app/assets/javascripts/pipeline_new/components/pipeline_new_form.vue @@ -136,11 +136,11 @@ export default { variables() { return { fullPath: this.projectPath, - sha: this.refQueryParam, + ref: this.refQueryParam, }; }, - update(data) { - return data?.project?.ciConfigVariables || []; + update({ project }) { + return project?.ciConfigVariables || []; }, result({ data }) { const predefinedVars = data?.project?.ciConfigVariables || []; @@ -149,10 +149,9 @@ export default { predefinedVars.forEach(({ description, key, value }) => { if (description) { - const valueOptions = value.includes('[') ? JSON.parse(value) : [value]; - [params[key]] = valueOptions; + [params[key]] = value; descriptions[key] = description; - this.predefinedVariables[key] = valueOptions; + this.predefinedVariables[key] = value; } }); diff --git a/app/assets/javascripts/pipeline_new/graphql/queries/ci_config_variables.graphql b/app/assets/javascripts/pipeline_new/graphql/queries/ci_config_variables.graphql index 4a3949daa51c23..1188016242ae5f 100644 --- a/app/assets/javascripts/pipeline_new/graphql/queries/ci_config_variables.graphql +++ b/app/assets/javascripts/pipeline_new/graphql/queries/ci_config_variables.graphql @@ -1,7 +1,7 @@ -query ciConfigVariables($fullPath: ID!, $sha: String!) { +query ciConfigVariables($fullPath: ID!, $ref: String!) { project(fullPath: $fullPath) { id - ciConfigVariables(sha: $sha) { + ciConfigVariables(sha: $ref) { description key value diff --git a/spec/frontend/pipeline_new/components/pipeline_new_form_spec.js b/spec/frontend/pipeline_new/components/pipeline_new_form_spec.js index afe3ad197247f3..8a8a40cd5a5cab 100644 --- a/spec/frontend/pipeline_new/components/pipeline_new_form_spec.js +++ b/spec/frontend/pipeline_new/components/pipeline_new_form_spec.js @@ -117,6 +117,7 @@ describe('Pipeline New Form', () => { afterEach(() => { mock.restore(); + wrapper.destroy(); }); describe('Form', () => { @@ -243,6 +244,22 @@ describe('Pipeline New Form', () => { expect(findKeyInputs().at(0).element.value).toBe('deploy_var'); expect(findVariableRows().length).toBe(2); }); + + it('skips query call when form variables are already cached', async () => { + await selectBranch('main'); + await changeKeyInputValue(0, 'build_var'); + + expect(mockCiConfigVariables).toHaveBeenCalledTimes(1); + + await selectBranch('branch-1'); + + expect(mockCiConfigVariables).toHaveBeenCalledTimes(2); + + // no additional call since `main` form values have been cached + await selectBranch('main'); + + expect(mockCiConfigVariables).toHaveBeenCalledTimes(2); + }); }); describe('when yml defines a variable', () => { @@ -265,13 +282,13 @@ describe('Pipeline New Form', () => { }); it('multi-line strings are added to the value field without removing line breaks', () => { - expect(findValueInputs().at(1).element.value).toBe(mockYamlVariables[1].value); + expect(findValueInputs().at(1).element.value).toBe(mockYamlVariables[1].value[0]); }); it('multiple predefined values are rendered as a dropdown', () => { const dropdown = findValueDropdowns().at(0); const dropdownItems = findValueDropdownItems(dropdown); - const yamlValue = JSON.parse(mockYamlVariables[2].value); + const yamlValue = mockYamlVariables[2].value; expect(dropdownItems.at(0).text()).toBe(yamlValue[0]); expect(dropdownItems.at(1).text()).toBe(yamlValue[1]); @@ -280,7 +297,7 @@ describe('Pipeline New Form', () => { it('variables with multiple predefined values sets the first option as the default', () => { const dropdown = findValueDropdowns().at(0); - const yamlValue = JSON.parse(mockYamlVariables[2].value); + const yamlValue = mockYamlVariables[2].value; expect(dropdown.props('text')).toBe(yamlValue[0]); }); @@ -299,7 +316,7 @@ describe('Pipeline New Form', () => { it('displays a variable from yml', () => { expect(findKeyInputs().at(0).element.value).toBe(mockYamlVariables[0].key); - expect(findValueInputs().at(0).element.value).toBe(mockYamlVariables[0].value); + expect(findValueInputs().at(0).element.value).toBe(mockYamlVariables[0].value[0]); }); it('displays a variable from provided query params', () => { diff --git a/spec/frontend/pipeline_new/mock_data.js b/spec/frontend/pipeline_new/mock_data.js index 362b352b61165e..609e6f4e2d6ab2 100644 --- a/spec/frontend/pipeline_new/mock_data.js +++ b/spec/frontend/pipeline_new/mock_data.js @@ -70,19 +70,20 @@ export const mockYamlVariables = [ { description: 'This is a variable with a value.', key: 'VAR_WITH_VALUE', - value: 'test_value', + value: ['test_value'], }, { description: 'This is a variable with a multi-line value.', key: 'VAR_WITH_MULTILINE', - value: `this is - a multiline - value`, + value: [ + `this is + a multiline value`, + ], }, { description: 'This is a variable with predefined values.', key: 'VAR_WITH_OPTIONS', - value: '["development", "staging", "production"]', + value: ['development', 'staging', 'production'], }, ]; @@ -90,19 +91,20 @@ export const mockYamlVariablesWithoutDesc = [ { description: 'This is a variable with a value.', key: 'VAR_WITH_VALUE', - value: 'test_value', + value: ['test_value'], }, { description: null, key: 'VAR_WITH_MULTILINE', - value: `this is - a multiline - value`, + value: [ + `this is + a multiline value`, + ], }, { description: null, key: 'VAR_WITH_OPTIONS', - value: '["development", "staging", "production"]', + value: ['development', 'staging', 'production'], }, ]; -- GitLab From 33630c9e2cb31ac38d91f6dac13810134e0533fd Mon Sep 17 00:00:00 2001 From: mgandres Date: Wed, 12 Oct 2022 19:01:18 +0800 Subject: [PATCH 3/3] Accommodate new valueOptions field If there are multiple options available, the backend will return them through the `valueOptions` field and it will be rendered as the dropdown options for the variable. --- .../components/pipeline_new_form.vue | 12 +++++----- .../queries/ci_config_variables.graphql | 1 + .../components/pipeline_new_form_spec.js | 16 +++++++------- spec/frontend/pipeline_new/mock_data.js | 22 ++++++++++--------- 4 files changed, 27 insertions(+), 24 deletions(-) diff --git a/app/assets/javascripts/pipeline_new/components/pipeline_new_form.vue b/app/assets/javascripts/pipeline_new/components/pipeline_new_form.vue index dc08d5339ba270..a9af1181027d45 100644 --- a/app/assets/javascripts/pipeline_new/components/pipeline_new_form.vue +++ b/app/assets/javascripts/pipeline_new/components/pipeline_new_form.vue @@ -118,7 +118,7 @@ export default { form: {}, errorTitle: null, error: null, - predefinedVariables: {}, + predefinedValueOptions: {}, warnings: [], totalWarnings: 0, isWarningDismissed: false, @@ -147,11 +147,11 @@ export default { const params = {}; const descriptions = {}; - predefinedVars.forEach(({ description, key, value }) => { + predefinedVars.forEach(({ description, key, value, valueOptions }) => { if (description) { - [params[key]] = value; + params[key] = value; descriptions[key] = description; - this.predefinedVariables[key] = value; + this.predefinedValueOptions[key] = valueOptions; } }); @@ -255,7 +255,7 @@ export default { }); }, shouldShowValuesDropdown(key) { - return this.predefinedVariables[key] && this.predefinedVariables[key].length > 1; + return this.predefinedValueOptions[key]?.length > 1; }, removeVariable(index) { this.variables.splice(index, 1); @@ -402,7 +402,7 @@ export default { data-testid="pipeline-form-ci-variable-value-dropdown" > { }); it('multi-line strings are added to the value field without removing line breaks', () => { - expect(findValueInputs().at(1).element.value).toBe(mockYamlVariables[1].value[0]); + expect(findValueInputs().at(1).element.value).toBe(mockYamlVariables[1].value); }); it('multiple predefined values are rendered as a dropdown', () => { const dropdown = findValueDropdowns().at(0); const dropdownItems = findValueDropdownItems(dropdown); - const yamlValue = mockYamlVariables[2].value; + const { valueOptions } = mockYamlVariables[2]; - expect(dropdownItems.at(0).text()).toBe(yamlValue[0]); - expect(dropdownItems.at(1).text()).toBe(yamlValue[1]); - expect(dropdownItems.at(2).text()).toBe(yamlValue[2]); + expect(dropdownItems.at(0).text()).toBe(valueOptions[0]); + expect(dropdownItems.at(1).text()).toBe(valueOptions[1]); + expect(dropdownItems.at(2).text()).toBe(valueOptions[2]); }); it('variables with multiple predefined values sets the first option as the default', () => { const dropdown = findValueDropdowns().at(0); - const yamlValue = mockYamlVariables[2].value; + const { valueOptions } = mockYamlVariables[2]; - expect(dropdown.props('text')).toBe(yamlValue[0]); + expect(dropdown.props('text')).toBe(valueOptions[0]); }); }); @@ -316,7 +316,7 @@ describe('Pipeline New Form', () => { it('displays a variable from yml', () => { expect(findKeyInputs().at(0).element.value).toBe(mockYamlVariables[0].key); - expect(findValueInputs().at(0).element.value).toBe(mockYamlVariables[0].value[0]); + expect(findValueInputs().at(0).element.value).toBe(mockYamlVariables[0].value); }); it('displays a variable from provided query params', () => { diff --git a/spec/frontend/pipeline_new/mock_data.js b/spec/frontend/pipeline_new/mock_data.js index 609e6f4e2d6ab2..e95a65171fcf30 100644 --- a/spec/frontend/pipeline_new/mock_data.js +++ b/spec/frontend/pipeline_new/mock_data.js @@ -70,20 +70,21 @@ export const mockYamlVariables = [ { description: 'This is a variable with a value.', key: 'VAR_WITH_VALUE', - value: ['test_value'], + value: 'test_value', + valueOptions: null, }, { description: 'This is a variable with a multi-line value.', key: 'VAR_WITH_MULTILINE', - value: [ - `this is + value: `this is a multiline value`, - ], + valueOptions: null, }, { description: 'This is a variable with predefined values.', key: 'VAR_WITH_OPTIONS', - value: ['development', 'staging', 'production'], + value: 'development', + valueOptions: ['development', 'staging', 'production'], }, ]; @@ -91,20 +92,21 @@ export const mockYamlVariablesWithoutDesc = [ { description: 'This is a variable with a value.', key: 'VAR_WITH_VALUE', - value: ['test_value'], + value: 'test_value', + valueOptions: null, }, { description: null, key: 'VAR_WITH_MULTILINE', - value: [ - `this is + value: `this is a multiline value`, - ], + valueOptions: null, }, { description: null, key: 'VAR_WITH_OPTIONS', - value: ['development', 'staging', 'production'], + value: 'development', + valueOptions: ['development', 'staging', 'production'], }, ]; -- GitLab