diff options
52 files changed, 1143 insertions, 581 deletions
diff --git a/app/assets/javascripts/ci_variable_list/ajax_variable_list.js b/app/assets/javascripts/ci_variable_list/ajax_variable_list.js new file mode 100644 index 00000000000..76f93e5c6bd --- /dev/null +++ b/app/assets/javascripts/ci_variable_list/ajax_variable_list.js @@ -0,0 +1,116 @@ +import _ from 'underscore'; +import axios from '../lib/utils/axios_utils'; +import { s__ } from '../locale'; +import Flash from '../flash'; +import { convertPermissionToBoolean } from '../lib/utils/common_utils'; +import statusCodes from '../lib/utils/http_status'; +import VariableList from './ci_variable_list'; + +function generateErrorBoxContent(errors) { + const errorList = [].concat(errors).map(errorString => ` + <li> + ${_.escape(errorString)} + </li> + `); + + return ` + <p> + ${s__('CiVariable|Validation failed')} + </p> + <ul> + ${errorList.join('')} + </ul> + `; +} + +// Used for the variable list on CI/CD projects/groups settings page +export default class AjaxVariableList { + constructor({ + container, + saveButton, + errorBox, + formField = 'variables', + saveEndpoint, + }) { + this.container = container; + this.saveButton = saveButton; + this.errorBox = errorBox; + this.saveEndpoint = saveEndpoint; + + this.variableList = new VariableList({ + container: this.container, + formField, + }); + + this.bindEvents(); + this.variableList.init(); + } + + bindEvents() { + this.saveButton.addEventListener('click', this.onSaveClicked.bind(this)); + } + + onSaveClicked() { + const loadingIcon = this.saveButton.querySelector('.js-secret-variables-save-loading-icon'); + loadingIcon.classList.toggle('hide', false); + this.errorBox.classList.toggle('hide', true); + // We use this to prevent a user from changing a key before we have a chance + // to match it up in `updateRowsWithPersistedVariables` + this.variableList.toggleEnableRow(false); + + return axios.patch(this.saveEndpoint, { + variables_attributes: this.variableList.getAllData(), + }, { + // We want to be able to process the `res.data` from a 400 error response + // and print the validation messages such as duplicate variable keys + validateStatus: status => ( + status >= statusCodes.OK && + status < statusCodes.MULTIPLE_CHOICES + ) || + status === statusCodes.BAD_REQUEST, + }) + .then((res) => { + loadingIcon.classList.toggle('hide', true); + this.variableList.toggleEnableRow(true); + + if (res.status === statusCodes.OK && res.data) { + this.updateRowsWithPersistedVariables(res.data.variables); + } else if (res.status === statusCodes.BAD_REQUEST) { + // Validation failed + this.errorBox.innerHTML = generateErrorBoxContent(res.data); + this.errorBox.classList.toggle('hide', false); + } + }) + .catch(() => { + loadingIcon.classList.toggle('hide', true); + this.variableList.toggleEnableRow(true); + Flash(s__('CiVariable|Error occured while saving variables')); + }); + } + + updateRowsWithPersistedVariables(persistedVariables = []) { + const persistedVariableMap = [].concat(persistedVariables).reduce((variableMap, variable) => ({ + ...variableMap, + [variable.key]: variable, + }), {}); + + this.container.querySelectorAll('.js-row').forEach((row) => { + // If we submitted a row that was destroyed, remove it so we don't try + // to destroy it again which would cause a BE error + const destroyInput = row.querySelector('.js-ci-variable-input-destroy'); + if (convertPermissionToBoolean(destroyInput.value)) { + row.remove(); + // Update the ID input so any future edits and `_destroy` will apply on the BE + } else { + const key = row.querySelector('.js-ci-variable-input-key').value; + const persistedVariable = persistedVariableMap[key]; + + if (persistedVariable) { + // eslint-disable-next-line no-param-reassign + row.querySelector('.js-ci-variable-input-id').value = persistedVariable.id; + row.setAttribute('data-is-persisted', 'true'); + } + } + }); + } +} diff --git a/app/assets/javascripts/ci_variable_list/ci_variable_list.js b/app/assets/javascripts/ci_variable_list/ci_variable_list.js index e46478ddb98..d91789c2192 100644 --- a/app/assets/javascripts/ci_variable_list/ci_variable_list.js +++ b/app/assets/javascripts/ci_variable_list/ci_variable_list.js @@ -11,7 +11,7 @@ function createEnvironmentItem(value) { return { title: value === '*' ? ALL_ENVIRONMENTS_STRING : value, id: value, - text: value, + text: value === '*' ? s__('CiVariable|* (All environments)') : value, }; } @@ -41,11 +41,11 @@ export default class VariableList { selector: '.js-ci-variable-input-protected', default: 'true', }, - environment: { + environment_scope: { // We can't use a `.js-` class here because // gl_dropdown replaces the <input> and doesn't copy over the class // See https://gitlab.com/gitlab-org/gitlab-ce/issues/42458 - selector: `input[name="${this.formField}[variables_attributes][][environment]"]`, + selector: `input[name="${this.formField}[variables_attributes][][environment_scope]"]`, default: '*', }, _destroy: { @@ -104,12 +104,15 @@ export default class VariableList { setupToggleButtons($row[0]); + // Reset the resizable textarea + $row.find(this.inputMap.value.selector).css('height', ''); + const $environmentSelect = $row.find('.js-variable-environment-toggle'); if ($environmentSelect.length) { const createItemDropdown = new CreateItemDropdown({ $dropdown: $environmentSelect, defaultToggleLabel: ALL_ENVIRONMENTS_STRING, - fieldName: `${this.formField}[variables_attributes][][environment]`, + fieldName: `${this.formField}[variables_attributes][][environment_scope]`, getData: (term, callback) => callback(this.getEnvironmentValues()), createNewItemFromValue: createEnvironmentItem, onSelect: () => { @@ -117,7 +120,7 @@ export default class VariableList { // so they have the new value we just picked this.refreshDropdownData(); - $row.find(this.inputMap.environment.selector).trigger('trigger-change'); + $row.find(this.inputMap.environment_scope.selector).trigger('trigger-change'); }, }); @@ -143,7 +146,8 @@ export default class VariableList { $row.after($rowClone); } - removeRow($row) { + removeRow(row) { + const $row = $(row); const isPersisted = convertPermissionToBoolean($row.attr('data-is-persisted')); if (isPersisted) { @@ -155,6 +159,10 @@ export default class VariableList { } else { $row.remove(); } + + // Refresh the other dropdowns in the variable list + // so any value with the variable deleted is gone + this.refreshDropdownData(); } checkIfRowTouched($row) { @@ -165,6 +173,11 @@ export default class VariableList { }); } + toggleEnableRow(isEnabled = true) { + this.$container.find(this.inputMap.key.selector).attr('disabled', !isEnabled); + this.$container.find('.js-row-remove-button').attr('disabled', !isEnabled); + } + getAllData() { // Ignore the last empty row because we don't want to try persist // a blank variable and run into validation problems. @@ -185,7 +198,7 @@ export default class VariableList { } getEnvironmentValues() { - const valueMap = this.$container.find(this.inputMap.environment.selector).toArray() + const valueMap = this.$container.find(this.inputMap.environment_scope.selector).toArray() .reduce((prevValueMap, envInput) => ({ ...prevValueMap, [envInput.value]: envInput.value, diff --git a/app/assets/javascripts/commons/polyfills/element.js b/app/assets/javascripts/commons/polyfills/element.js index 9a1f73bf2ac..b593bde6aa2 100644 --- a/app/assets/javascripts/commons/polyfills/element.js +++ b/app/assets/javascripts/commons/polyfills/element.js @@ -18,3 +18,22 @@ Element.prototype.matches = Element.prototype.matches || while (i >= 0 && elms.item(i) !== this) { i -= 1; } return i > -1; }; + +// From the polyfill on MDN, https://developer.mozilla.org/en-US/docs/Web/API/ChildNode/remove#Polyfill +((arr) => { + arr.forEach((item) => { + if (Object.prototype.hasOwnProperty.call(item, 'remove')) { + return; + } + Object.defineProperty(item, 'remove', { + configurable: true, + enumerable: true, + writable: true, + value: function remove() { + if (this.parentNode !== null) { + this.parentNode.removeChild(this); + } + }, + }); + }); +})([Element.prototype, CharacterData.prototype, DocumentType.prototype]); diff --git a/app/assets/javascripts/lib/utils/http_status.js b/app/assets/javascripts/lib/utils/http_status.js index 625e53ee9de..bb151929431 100644 --- a/app/assets/javascripts/lib/utils/http_status.js +++ b/app/assets/javascripts/lib/utils/http_status.js @@ -6,4 +6,6 @@ export default { ABORTED: 0, NO_CONTENT: 204, OK: 200, + MULTIPLE_CHOICES: 300, + BAD_REQUEST: 400, }; diff --git a/app/assets/javascripts/pages/groups/settings/ci_cd/show/index.js b/app/assets/javascripts/pages/groups/settings/ci_cd/show/index.js index f26c7360fbe..ad79f7e09ac 100644 --- a/app/assets/javascripts/pages/groups/settings/ci_cd/show/index.js +++ b/app/assets/javascripts/pages/groups/settings/ci_cd/show/index.js @@ -1,11 +1,12 @@ -import SecretValues from '~/behaviors/secret_values'; +import AjaxVariableList from '~/ci_variable_list/ajax_variable_list'; export default () => { - const secretVariableTable = document.querySelector('.js-secret-variable-table'); - if (secretVariableTable) { - const secretVariableTableValues = new SecretValues({ - container: secretVariableTable, - }); - secretVariableTableValues.init(); - } + const variableListEl = document.querySelector('.js-ci-variable-list-section'); + // eslint-disable-next-line no-new + new AjaxVariableList({ + container: variableListEl, + saveButton: variableListEl.querySelector('.js-secret-variables-save-button'), + errorBox: variableListEl.querySelector('.js-ci-variable-error-box'), + saveEndpoint: variableListEl.dataset.saveEndpoint, + }); }; diff --git a/app/assets/javascripts/pages/projects/settings/ci_cd/show/index.js b/app/assets/javascripts/pages/projects/settings/ci_cd/show/index.js index 18dc1dc03a5..a563d0f9961 100644 --- a/app/assets/javascripts/pages/projects/settings/ci_cd/show/index.js +++ b/app/assets/javascripts/pages/projects/settings/ci_cd/show/index.js @@ -1,9 +1,11 @@ import initSettingsPanels from '~/settings_panels'; import SecretValues from '~/behaviors/secret_values'; +import AjaxVariableList from '~/ci_variable_list/ajax_variable_list'; export default function () { // Initialize expandable settings panels initSettingsPanels(); + const runnerToken = document.querySelector('.js-secret-runner-token'); if (runnerToken) { const runnerTokenSecretValue = new SecretValues({ @@ -12,11 +14,12 @@ export default function () { runnerTokenSecretValue.init(); } - const secretVariableTable = document.querySelector('.js-secret-variable-table'); - if (secretVariableTable) { - const secretVariableTableValues = new SecretValues({ - container: secretVariableTable, - }); - secretVariableTableValues.init(); - } + const variableListEl = document.querySelector('.js-ci-variable-list-section'); + // eslint-disable-next-line no-new + new AjaxVariableList({ + container: variableListEl, + saveButton: variableListEl.querySelector('.js-secret-variables-save-button'), + errorBox: variableListEl.querySelector('.js-ci-variable-error-box'), + saveEndpoint: variableListEl.dataset.saveEndpoint, + }); } diff --git a/app/assets/stylesheets/framework/ci_variable_list.scss b/app/assets/stylesheets/framework/ci_variable_list.scss index 8f654ab363c..5fe835dd8f9 100644 --- a/app/assets/stylesheets/framework/ci_variable_list.scss +++ b/app/assets/stylesheets/framework/ci_variable_list.scss @@ -8,7 +8,11 @@ .ci-variable-row { display: flex; - align-items: flex-end; + align-items: flex-start; + + @media (max-width: $screen-xs-max) { + align-items: flex-end; + } &:not(:last-child) { margin-bottom: $gl-btn-padding; @@ -41,6 +45,7 @@ .ci-variable-row-body { display: flex; + align-items: flex-start; width: 100%; @media (max-width: $screen-xs-max) { @@ -65,6 +70,8 @@ flex: 0 1 auto; display: flex; align-items: center; + padding-top: 5px; + padding-bottom: 5px; } .ci-variable-row-remove-button { @@ -85,4 +92,8 @@ outline: none; color: $gl-text-color; } + + &[disabled] { + color: $gl-text-color-disabled; + } } diff --git a/app/controllers/groups/variables_controller.rb b/app/controllers/groups/variables_controller.rb index 10038ff3ad9..913e13bf734 100644 --- a/app/controllers/groups/variables_controller.rb +++ b/app/controllers/groups/variables_controller.rb @@ -1,60 +1,43 @@ module Groups class VariablesController < Groups::ApplicationController - before_action :variable, only: [:show, :update, :destroy] before_action :authorize_admin_build! - def index - redirect_to group_settings_ci_cd_path(group) - end - def show + respond_to do |format| + format.json do + render status: :ok, json: { variables: GroupVariableSerializer.new.represent(@group.variables) } + end + end end def update - if variable.update(variable_params) - redirect_to group_variables_path(group), - notice: 'Variable was successfully updated.' + if @group.update(group_variables_params) + respond_to do |format| + format.json { return render_group_variables } + end else - render "show" + respond_to do |format| + format.json { render_error } + end end end - def create - @variable = group.variables.create(variable_params) - .present(current_user: current_user) + private - if @variable.persisted? - redirect_to group_settings_ci_cd_path(group), - notice: 'Variable was successfully created.' - else - render "show" - end + def render_group_variables + render status: :ok, json: { variables: GroupVariableSerializer.new.represent(@group.variables) } end - def destroy - if variable.destroy - redirect_to group_settings_ci_cd_path(group), - status: 302, - notice: 'Variable was successfully removed.' - else - redirect_to group_settings_ci_cd_path(group), - status: 302, - notice: 'Failed to remove the variable.' - end + def render_error + render status: :bad_request, json: @group.errors.full_messages end - private - - def variable_params - params.require(:variable).permit(*variable_params_attributes) + def group_variables_params + params.permit(variables_attributes: [*variable_params_attributes]) end def variable_params_attributes - %i[key value protected] - end - - def variable - @variable ||= group.variables.find(params[:id]).present(current_user: current_user) + %i[id key value protected _destroy] end def authorize_admin_build! diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb index 6a825137564..7eb509e2e64 100644 --- a/app/controllers/projects/variables_controller.rb +++ b/app/controllers/projects/variables_controller.rb @@ -1,60 +1,41 @@ class Projects::VariablesController < Projects::ApplicationController - before_action :variable, only: [:show, :update, :destroy] before_action :authorize_admin_build! - layout 'project_settings' - - def index - redirect_to project_settings_ci_cd_path(@project) - end - def show + respond_to do |format| + format.json do + render status: :ok, json: { variables: VariableSerializer.new.represent(@project.variables) } + end + end end def update - if variable.update(variable_params) - redirect_to project_variables_path(project), - notice: 'Variable was successfully updated.' + if @project.update(variables_params) + respond_to do |format| + format.json { return render_variables } + end else - render "show" + respond_to do |format| + format.json { render_error } + end end end - def create - @variable = project.variables.create(variable_params) - .present(current_user: current_user) + private - if @variable.persisted? - redirect_to project_settings_ci_cd_path(project), - notice: 'Variable was successfully created.' - else - render "show" - end + def render_variables + render status: :ok, json: { variables: VariableSerializer.new.represent(@project.variables) } end - def destroy - if variable.destroy - redirect_to project_settings_ci_cd_path(project), - status: 302, - notice: 'Variable was successfully removed.' - else - redirect_to project_settings_ci_cd_path(project), - status: 302, - notice: 'Failed to remove the variable.' - end + def render_error + render status: :bad_request, json: @project.errors.full_messages end - private - - def variable_params - params.require(:variable).permit(*variable_params_attributes) + def variables_params + params.permit(variables_attributes: [*variable_params_attributes]) end def variable_params_attributes %i[id key value protected _destroy] end - - def variable - @variable ||= project.variables.find(params[:id]).present(current_user: current_user) - end end diff --git a/app/models/group.rb b/app/models/group.rb index 5b7f1b38612..75bf013ecd2 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -31,9 +31,12 @@ class Group < Namespace has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + accepts_nested_attributes_for :variables, allow_destroy: true + validate :visibility_level_allowed_by_projects validate :visibility_level_allowed_by_sub_groups validate :visibility_level_allowed_by_parent + validates :variables, variable_duplicates: true validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 } diff --git a/app/models/project.rb b/app/models/project.rb index 3edb6109fb8..34fb39dd28e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -260,6 +260,7 @@ class Project < ActiveRecord::Base validates :repository_storage, presence: true, inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } } + validates :variables, variable_duplicates: true has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent diff --git a/app/presenters/ci/group_variable_presenter.rb b/app/presenters/ci/group_variable_presenter.rb index 81fea106a5c..98d68bc7a83 100644 --- a/app/presenters/ci/group_variable_presenter.rb +++ b/app/presenters/ci/group_variable_presenter.rb @@ -7,19 +7,15 @@ module Ci end def form_path - if variable.persisted? - group_variable_path(group, variable) - else - group_variables_path(group) - end + group_settings_ci_cd_path(group) end def edit_path - group_variable_path(group, variable) + group_variables_path(group) end def delete_path - group_variable_path(group, variable) + group_variables_path(group) end end end diff --git a/app/presenters/ci/variable_presenter.rb b/app/presenters/ci/variable_presenter.rb index 5d7998393a6..96159f88c59 100644 --- a/app/presenters/ci/variable_presenter.rb +++ b/app/presenters/ci/variable_presenter.rb @@ -7,19 +7,15 @@ module Ci end def form_path - if variable.persisted? - project_variable_path(project, variable) - else - project_variables_path(project) - end + project_settings_ci_cd_path(project) end def edit_path - project_variable_path(project, variable) + project_variables_path(project) end def delete_path - project_variable_path(project, variable) + project_variables_path(project) end end end diff --git a/app/serializers/group_variable_entity.rb b/app/serializers/group_variable_entity.rb new file mode 100644 index 00000000000..62cf0b21e1e --- /dev/null +++ b/app/serializers/group_variable_entity.rb @@ -0,0 +1,7 @@ +class GroupVariableEntity < Grape::Entity + expose :id + expose :key + expose :value + + expose :protected?, as: :protected +end diff --git a/app/serializers/group_variable_serializer.rb b/app/serializers/group_variable_serializer.rb new file mode 100644 index 00000000000..8f8205924aa --- /dev/null +++ b/app/serializers/group_variable_serializer.rb @@ -0,0 +1,3 @@ +class GroupVariableSerializer < BaseSerializer + entity GroupVariableEntity +end diff --git a/app/serializers/variable_entity.rb b/app/serializers/variable_entity.rb new file mode 100644 index 00000000000..d576745c073 --- /dev/null +++ b/app/serializers/variable_entity.rb @@ -0,0 +1,7 @@ +class VariableEntity < Grape::Entity + expose :id + expose :key + expose :value + + expose :protected?, as: :protected +end diff --git a/app/serializers/variable_serializer.rb b/app/serializers/variable_serializer.rb new file mode 100644 index 00000000000..32ae82ab51c --- /dev/null +++ b/app/serializers/variable_serializer.rb @@ -0,0 +1,3 @@ +class VariableSerializer < BaseSerializer + entity VariableEntity +end diff --git a/app/views/ci/variables/_content.html.haml b/app/views/ci/variables/_content.html.haml index fbfe3e56588..d355e7799df 100644 --- a/app/views/ci/variables/_content.html.haml +++ b/app/views/ci/variables/_content.html.haml @@ -1,3 +1 @@ -%p.append-bottom-default - Variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. - You can use variables for passwords, secret keys, or whatever you want. += _('Variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. You can use variables for passwords, secret keys, or whatever you want.') diff --git a/app/views/ci/variables/_form.html.haml b/app/views/ci/variables/_form.html.haml deleted file mode 100644 index eebd0955c80..00000000000 --- a/app/views/ci/variables/_form.html.haml +++ /dev/null @@ -1,19 +0,0 @@ -= form_for @variable, as: :variable, url: @variable.form_path do |f| - = form_errors(@variable) - - .form-group - = f.label :key, "Key", class: "label-light" - = f.text_field :key, class: "form-control", placeholder: @variable.placeholder, required: true - .form-group - = f.label :value, "Value", class: "label-light" - = f.text_area :value, class: "form-control", placeholder: @variable.placeholder - .form-group - .checkbox - = f.label :protected do - = f.check_box :protected - %strong Protected - .help-block - This variable will be passed only to pipelines running on protected branches and tags - = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'protected-secret-variables'), target: '_blank' - - = f.submit btn_text, class: "btn btn-save" diff --git a/app/views/ci/variables/_index.html.haml b/app/views/ci/variables/_index.html.haml index 6e399fc7392..e402801a776 100644 --- a/app/views/ci/variables/_index.html.haml +++ b/app/views/ci/variables/_index.html.haml @@ -1,16 +1,20 @@ -.row.prepend-top-default.append-bottom-default - .col-lg-12 - %h5.prepend-top-0 - Add a variable - = render "ci/variables/form", btn_text: "Add new variable" - %hr - %h5.prepend-top-0 - Your variables (#{@variables.size}) - - if @variables.empty? - %p.settings-message.text-center.append-bottom-0 - No variables found, add one with the form above. - - else - .js-secret-variable-table - = render "ci/variables/table" - %button.btn.btn-info.js-secret-value-reveal-button{ data: { secret_reveal_status: 'false' } } +- save_endpoint = local_assigns.fetch(:save_endpoint, nil) + +.row + .col-lg-12.js-ci-variable-list-section{ data: { save_endpoint: save_endpoint } } + .hide.alert.alert-danger.js-ci-variable-error-box + + %ul.ci-variable-list + - @variables.each.each do |variable| + = render 'ci/variables/variable_row', form_field: 'variables', variable: variable + = render 'ci/variables/variable_row', form_field: 'variables' + .prepend-top-20 + %button.btn.btn-success.js-secret-variables-save-button{ type: 'button' } + %span.hide.js-secret-variables-save-loading-icon + = icon('spinner spin') + = _('Save variables') + %button.btn.btn-info.btn-inverted.prepend-left-10.js-secret-value-reveal-button{ type: 'button', data: { secret_reveal_status: "#{@variables.size == 0}" } } + - if @variables.size == 0 + = n_('Hide value', 'Hide values', @variables.size) + - else = n_('Reveal value', 'Reveal values', @variables.size) diff --git a/app/views/ci/variables/_show.html.haml b/app/views/ci/variables/_show.html.haml deleted file mode 100644 index 6d75ae96124..00000000000 --- a/app/views/ci/variables/_show.html.haml +++ /dev/null @@ -1,9 +0,0 @@ -- page_title "Variables" - -.row.prepend-top-default.append-bottom-default - .col-lg-3 - = render "ci/variables/content" - .col-lg-9 - %h4.prepend-top-0 - Update variable - = render "ci/variables/form", btn_text: "Save variable" diff --git a/app/views/ci/variables/_table.html.haml b/app/views/ci/variables/_table.html.haml deleted file mode 100644 index 2298930d0c7..00000000000 --- a/app/views/ci/variables/_table.html.haml +++ /dev/null @@ -1,32 +0,0 @@ -.table-responsive.variables-table - %table.table - %colgroup - %col - %col - %col - %col{ width: 100 } - %thead - %th Key - %th Value - %th Protected - %th - %tbody - - @variables.each do |variable| - - if variable.id? - %tr - %td.variable-key= variable.key - %td.variable-value - %span.js-secret-value-placeholder - = '*' * 6 - %span.hide.js-secret-value - = variable.value - %td.variable-protected= Gitlab::Utils.boolean_to_yes_no(variable.protected) - %td.variable-menu - = link_to variable.edit_path, class: "btn btn-transparent btn-variable-edit" do - %span.sr-only - Update - = icon("pencil") - = link_to variable.delete_path, class: "btn btn-transparent btn-variable-delete", method: :delete, data: { confirm: "Are you sure?" } do - %span.sr-only - Remove - = icon("trash") diff --git a/app/views/groups/settings/ci_cd/show.html.haml b/app/views/groups/settings/ci_cd/show.html.haml index 472da2a6a72..dd82922ec55 100644 --- a/app/views/groups/settings/ci_cd/show.html.haml +++ b/app/views/groups/settings/ci_cd/show.html.haml @@ -1,4 +1,11 @@ - breadcrumb_title "CI / CD Settings" - page_title "CI / CD" -= render 'ci/variables/index' +%h4 + = _('Secret variables') + = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'secret-variables'), target: '_blank', rel: 'noopener noreferrer' + +%p + = render "ci/variables/content" + += render 'ci/variables/index', save_endpoint: group_variables_path diff --git a/app/views/groups/variables/show.html.haml b/app/views/groups/variables/show.html.haml deleted file mode 100644 index df533952b76..00000000000 --- a/app/views/groups/variables/show.html.haml +++ /dev/null @@ -1 +0,0 @@ -= render 'ci/variables/show' diff --git a/app/views/projects/settings/ci_cd/show.html.haml b/app/views/projects/settings/ci_cd/show.html.haml index 664a4554692..756f31f91d9 100644 --- a/app/views/projects/settings/ci_cd/show.html.haml +++ b/app/views/projects/settings/ci_cd/show.html.haml @@ -29,14 +29,14 @@ %section.settings.no-animate{ class: ('expanded' if expanded) } .settings-header %h4 - Secret variables - = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'secret-variables'), target: '_blank' + = _('Secret variables') + = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'secret-variables'), target: '_blank', rel: 'noopener noreferrer' %button.btn.js-settings-toggle = expanded ? 'Collapse' : 'Expand' - %p + %p.append-bottom-0 = render "ci/variables/content" .settings-content - = render 'ci/variables/index' + = render 'ci/variables/index', save_endpoint: project_variables_path(@project) %section.settings.no-animate{ class: ('expanded' if expanded) } .settings-header diff --git a/app/views/projects/variables/show.html.haml b/app/views/projects/variables/show.html.haml deleted file mode 100644 index df533952b76..00000000000 --- a/app/views/projects/variables/show.html.haml +++ /dev/null @@ -1 +0,0 @@ -= render 'ci/variables/show' diff --git a/changelogs/unreleased-ee/39118-dynamic-pipeline-variables-fe.yml b/changelogs/unreleased-ee/39118-dynamic-pipeline-variables-fe.yml new file mode 100644 index 00000000000..a38b447e345 --- /dev/null +++ b/changelogs/unreleased-ee/39118-dynamic-pipeline-variables-fe.yml @@ -0,0 +1,6 @@ +--- +title: Update CI/CD secret variables list to be dynamic and save without reloading + the page +merge_request: 4110 +author: +type: added diff --git a/config/routes/group.rb b/config/routes/group.rb index b17611d8623..7a4740a4df7 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -28,7 +28,7 @@ constraints(GroupUrlConstrainer.new) do resource :ci_cd, only: [:show], controller: 'ci_cd' end - resources :variables, only: [:index, :show, :update, :create, :destroy] + resource :variables, only: [:show, :update] resources :children, only: [:index] diff --git a/config/routes/project.rb b/config/routes/project.rb index bcaa68c8ce5..1912808f9c0 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -156,7 +156,8 @@ constraints(ProjectUrlConstrainer.new) do end end - resources :variables, only: [:index, :show, :update, :create, :destroy] + resource :variables, only: [:show, :update] + resources :triggers, only: [:index, :create, :edit, :update, :destroy] do member do post :take_ownership diff --git a/doc/ci/variables/img/secret_variables.png b/doc/ci/variables/img/secret_variables.png Binary files differindex f70935069d9..3c1aa361dc2 100644 --- a/doc/ci/variables/img/secret_variables.png +++ b/doc/ci/variables/img/secret_variables.png diff --git a/qa/qa/factory/resource/secret_variable.rb b/qa/qa/factory/resource/secret_variable.rb index 54ef4d8d964..af0fa8af2df 100644 --- a/qa/qa/factory/resource/secret_variable.rb +++ b/qa/qa/factory/resource/secret_variable.rb @@ -31,7 +31,7 @@ module QA page.fill_variable_key(key) page.fill_variable_value(value) - page.add_variable + page.save_variables end end end diff --git a/qa/qa/page/project/settings/secret_variables.rb b/qa/qa/page/project/settings/secret_variables.rb index e3bfbfcf080..fea4acb389a 100644 --- a/qa/qa/page/project/settings/secret_variables.rb +++ b/qa/qa/page/project/settings/secret_variables.rb @@ -5,49 +5,40 @@ module QA class SecretVariables < Page::Base include Common - view 'app/views/ci/variables/_table.html.haml' do - element :variable_key, '.variable-key' - element :variable_value, '.variable-value' + view 'app/views/ci/variables/_variable_row.html.haml' do + element :variable_key, '.js-ci-variable-input-key' + element :variable_value, '.js-ci-variable-input-value' end view 'app/views/ci/variables/_index.html.haml' do - element :add_new_variable, 'btn_text: "Add new variable"' - end - - view 'app/assets/javascripts/behaviors/secret_values.js' do - element :reveal_value, 'Reveal value' - element :hide_value, 'Hide value' + element :save_variables, '.js-secret-variables-save-button' end def fill_variable_key(key) - fill_in 'variable_key', with: key + page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do + page.find('.js-ci-variable-input-key').set(key) + end end def fill_variable_value(value) - fill_in 'variable_value', with: value + page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do + page.find('.js-ci-variable-input-value').set(value) + end end - def add_variable - click_on 'Add new variable' + def save_variables + click_button('Save variables') end def variable_key - page.find('.variable-key').text - end - - def variable_value - reveal_value do - page.find('.variable-value').text + page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do + page.find('.js-ci-variable-input-key').value end end - private - - def reveal_value - click_button('Reveal value') - - yield.tap do - click_button('Hide value') + def variable_value + page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do + page.find('.js-ci-variable-input-value').value end end end diff --git a/spec/controllers/groups/variables_controller_spec.rb b/spec/controllers/groups/variables_controller_spec.rb index 8ea98cd9e8f..39a36b92bb4 100644 --- a/spec/controllers/groups/variables_controller_spec.rb +++ b/spec/controllers/groups/variables_controller_spec.rb @@ -9,48 +9,27 @@ describe Groups::VariablesController do group.add_master(user) end - describe 'POST #create' do - context 'variable is valid' do - it 'shows a success flash message' do - post :create, group_id: group, variable: { key: "one", value: "two" } - - expect(flash[:notice]).to include 'Variable was successfully created.' - expect(response).to redirect_to(group_settings_ci_cd_path(group)) - end - end - - context 'variable is invalid' do - it 'renders show' do - post :create, group_id: group, variable: { key: "..one", value: "two" } + describe 'GET #show' do + let!(:variable) { create(:ci_group_variable, group: group) } - expect(response).to render_template("groups/variables/show") - end + subject do + get :show, group_id: group, format: :json end - end - - describe 'POST #update' do - let(:variable) { create(:ci_group_variable) } - context 'updating a variable with valid characters' do - before do - group.variables << variable - end - - it 'shows a success flash message' do - post :update, group_id: group, - id: variable.id, variable: { key: variable.key, value: 'two' } - - expect(flash[:notice]).to include 'Variable was successfully updated.' - expect(response).to redirect_to(group_variables_path(group)) - end + include_examples 'GET #show lists all variables' + end - it 'renders the action #show if the variable key is invalid' do - post :update, group_id: group, - id: variable.id, variable: { key: '?', value: variable.value } + describe 'PATCH #update' do + let!(:variable) { create(:ci_group_variable, group: group) } + let(:owner) { group } - expect(response).to have_gitlab_http_status(200) - expect(response).to render_template :show - end + subject do + patch :update, + group_id: group, + variables_attributes: variables_attributes, + format: :json end + + include_examples 'PATCH #update updates variables' end end diff --git a/spec/controllers/projects/variables_controller_spec.rb b/spec/controllers/projects/variables_controller_spec.rb index 9fde6544215..68019743be0 100644 --- a/spec/controllers/projects/variables_controller_spec.rb +++ b/spec/controllers/projects/variables_controller_spec.rb @@ -9,50 +9,28 @@ describe Projects::VariablesController do project.add_master(user) end - describe 'POST #create' do - context 'variable is valid' do - it 'shows a success flash message' do - post :create, namespace_id: project.namespace.to_param, project_id: project, - variable: { key: "one", value: "two" } - - expect(flash[:notice]).to include 'Variable was successfully created.' - expect(response).to redirect_to(project_settings_ci_cd_path(project)) - end - end - - context 'variable is invalid' do - it 'renders show' do - post :create, namespace_id: project.namespace.to_param, project_id: project, - variable: { key: "..one", value: "two" } + describe 'GET #show' do + let!(:variable) { create(:ci_variable, project: project) } - expect(response).to render_template("projects/variables/show") - end + subject do + get :show, namespace_id: project.namespace.to_param, project_id: project, format: :json end - end - - describe 'POST #update' do - let(:variable) { create(:ci_variable) } - context 'updating a variable with valid characters' do - before do - project.variables << variable - end - - it 'shows a success flash message' do - post :update, namespace_id: project.namespace.to_param, project_id: project, - id: variable.id, variable: { key: variable.key, value: 'two' } - - expect(flash[:notice]).to include 'Variable was successfully updated.' - expect(response).to redirect_to(project_variables_path(project)) - end + include_examples 'GET #show lists all variables' + end - it 'renders the action #show if the variable key is invalid' do - post :update, namespace_id: project.namespace.to_param, project_id: project, - id: variable.id, variable: { key: '?', value: variable.value } + describe 'PATCH #update' do + let!(:variable) { create(:ci_variable, project: project) } + let(:owner) { project } - expect(response).to have_gitlab_http_status(200) - expect(response).to render_template :show - end + subject do + patch :update, + namespace_id: project.namespace.to_param, + project_id: project, + variables_attributes: variables_attributes, + format: :json end + + include_examples 'PATCH #update updates variables' end end diff --git a/spec/features/group_variables_spec.rb b/spec/features/group_variables_spec.rb index e9b375f4c94..f7863807572 100644 --- a/spec/features/group_variables_spec.rb +++ b/spec/features/group_variables_spec.rb @@ -3,76 +3,15 @@ require 'spec_helper' feature 'Group variables', :js do let(:user) { create(:user) } let(:group) { create(:group) } + let!(:variable) { create(:ci_group_variable, key: 'test_key', value: 'test value', group: group) } + let(:page_path) { group_settings_ci_cd_path(group) } background do group.add_master(user) gitlab_sign_in(user) - end - - context 'when user creates a new variable' do - background do - visit group_settings_ci_cd_path(group) - fill_in 'variable_key', with: 'AAA' - fill_in 'variable_value', with: 'AAA123' - find(:css, "#variable_protected").set(true) - click_on 'Add new variable' - end - - scenario 'user sees the created variable' do - page.within('.variables-table') do - expect(find(".variable-key")).to have_content('AAA') - expect(find(".variable-value")).to have_content('******') - expect(find(".variable-protected")).to have_content('Yes') - end - click_on 'Reveal value' - page.within('.variables-table') do - expect(find(".variable-value")).to have_content('AAA123') - end - end - end - - context 'when user edits a variable' do - background do - create(:ci_group_variable, key: 'AAA', value: 'AAA123', protected: true, - group: group) - - visit group_settings_ci_cd_path(group) - page.within('.variable-menu') do - click_on 'Update' - end - - fill_in 'variable_key', with: 'BBB' - fill_in 'variable_value', with: 'BBB123' - find(:css, "#variable_protected").set(false) - click_on 'Save variable' - end - - scenario 'user sees the updated variable' do - page.within('.variables-table') do - expect(find(".variable-key")).to have_content('BBB') - expect(find(".variable-value")).to have_content('******') - expect(find(".variable-protected")).to have_content('No') - end - end + visit page_path end - context 'when user deletes a variable' do - background do - create(:ci_group_variable, key: 'BBB', value: 'BBB123', protected: false, - group: group) - - visit group_settings_ci_cd_path(group) - - page.within('.variable-menu') do - page.accept_alert 'Are you sure?' do - click_on 'Remove' - end - end - end - - scenario 'user does not see the deleted variable' do - expect(page).to have_no_css('.variables-table') - end - end + it_behaves_like 'variable list' end diff --git a/spec/features/project_variables_spec.rb b/spec/features/project_variables_spec.rb new file mode 100644 index 00000000000..0ba2224359a --- /dev/null +++ b/spec/features/project_variables_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +describe 'Project variables', :js do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:variable) { create(:ci_variable, key: 'test_key', value: 'test value') } + let(:page_path) { project_settings_ci_cd_path(project) } + + before do + sign_in(user) + project.add_master(user) + project.variables << variable + + visit page_path + end + + it_behaves_like 'variable list' +end diff --git a/spec/features/variables_spec.rb b/spec/features/variables_spec.rb deleted file mode 100644 index 79ca2b4bb4a..00000000000 --- a/spec/features/variables_spec.rb +++ /dev/null @@ -1,145 +0,0 @@ -require 'spec_helper' - -describe 'Project variables', :js do - let(:user) { create(:user) } - let(:project) { create(:project) } - let(:variable) { create(:ci_variable, key: 'test_key', value: 'test value') } - - before do - sign_in(user) - project.add_master(user) - project.variables << variable - - visit project_settings_ci_cd_path(project) - end - - it 'shows list of variables' do - page.within('.variables-table') do - expect(page).to have_content(variable.key) - end - end - - it 'adds new secret variable' do - fill_in('variable_key', with: 'key') - fill_in('variable_value', with: 'key value') - click_button('Add new variable') - - expect(page).to have_content('Variable was successfully created.') - page.within('.variables-table') do - expect(page).to have_content('key') - expect(page).to have_content('No') - end - end - - it 'adds empty variable' do - fill_in('variable_key', with: 'new_key') - fill_in('variable_value', with: '') - click_button('Add new variable') - - expect(page).to have_content('Variable was successfully created.') - page.within('.variables-table') do - expect(page).to have_content('new_key') - end - end - - it 'adds new protected variable' do - fill_in('variable_key', with: 'key') - fill_in('variable_value', with: 'value') - check('Protected') - click_button('Add new variable') - - expect(page).to have_content('Variable was successfully created.') - page.within('.variables-table') do - expect(page).to have_content('key') - expect(page).to have_content('Yes') - end - end - - it 'reveals and hides new variable' do - fill_in('variable_key', with: 'key') - fill_in('variable_value', with: 'key value') - click_button('Add new variable') - - page.within('.variables-table') do - expect(page).to have_content('key') - expect(page).to have_content('******') - end - - click_button('Reveal values') - - page.within('.variables-table') do - expect(page).to have_content('key') - expect(page).to have_content('key value') - end - - click_button('Hide values') - - page.within('.variables-table') do - expect(page).to have_content('key') - expect(page).to have_content('******') - end - end - - it 'deletes variable' do - page.within('.variables-table') do - accept_confirm { click_on 'Remove' } - end - - expect(page).not_to have_selector('variables-table') - end - - it 'edits variable' do - page.within('.variables-table') do - click_on 'Update' - end - - expect(page).to have_content('Update variable') - fill_in('variable_key', with: 'key') - fill_in('variable_value', with: 'key value') - click_button('Save variable') - - expect(page).to have_content('Variable was successfully updated.') - expect(project.variables(true).first.value).to eq('key value') - end - - it 'edits variable with empty value' do - page.within('.variables-table') do - click_on 'Update' - end - - expect(page).to have_content('Update variable') - fill_in('variable_value', with: '') - click_button('Save variable') - - expect(page).to have_content('Variable was successfully updated.') - expect(project.variables(true).first.value).to eq('') - end - - it 'edits variable to be protected' do - page.within('.variables-table') do - click_on 'Update' - end - - expect(page).to have_content('Update variable') - check('Protected') - click_button('Save variable') - - expect(page).to have_content('Variable was successfully updated.') - expect(project.variables(true).first).to be_protected - end - - it 'edits variable to be unprotected' do - project.variables.first.update(protected: true) - - page.within('.variables-table') do - click_on 'Update' - end - - expect(page).to have_content('Update variable') - uncheck('Protected') - click_button('Save variable') - - expect(page).to have_content('Variable was successfully updated.') - expect(project.variables(true).first).not_to be_protected - end -end diff --git a/spec/fixtures/api/schemas/variable.json b/spec/fixtures/api/schemas/variable.json new file mode 100644 index 00000000000..78977118b0a --- /dev/null +++ b/spec/fixtures/api/schemas/variable.json @@ -0,0 +1,16 @@ +{ + "type": "object", + "required": [ + "id", + "key", + "value", + "protected" + ], + "properties": { + "id": { "type": "integer" }, + "key": { "type": "string" }, + "value": { "type": "string" }, + "protected": { "type": "boolean" } + }, + "additionalProperties": false +} diff --git a/spec/fixtures/api/schemas/variables.json b/spec/fixtures/api/schemas/variables.json new file mode 100644 index 00000000000..8002f39a7b8 --- /dev/null +++ b/spec/fixtures/api/schemas/variables.json @@ -0,0 +1,11 @@ +{ + "type": "object", + "required": ["variables"], + "properties": { + "variables": { + "type": "array", + "items": { "$ref": "variable.json" } + } + }, + "additionalProperties": false +} diff --git a/spec/javascripts/ci_variable_list/ajax_variable_list_spec.js b/spec/javascripts/ci_variable_list/ajax_variable_list_spec.js new file mode 100644 index 00000000000..5b9cdceee71 --- /dev/null +++ b/spec/javascripts/ci_variable_list/ajax_variable_list_spec.js @@ -0,0 +1,189 @@ +import MockAdapter from 'axios-mock-adapter'; +import axios from '~/lib/utils/axios_utils'; +import AjaxFormVariableList from '~/ci_variable_list/ajax_variable_list'; + +const VARIABLE_PATCH_ENDPOINT = 'http://test.host/frontend-fixtures/builds-project/variables'; + +describe('AjaxFormVariableList', () => { + preloadFixtures('projects/ci_cd_settings.html.raw'); + preloadFixtures('projects/ci_cd_settings_with_variables.html.raw'); + + let container; + let saveButton; + let errorBox; + + let mock; + let ajaxVariableList; + + beforeEach(() => { + loadFixtures('projects/ci_cd_settings.html.raw'); + container = document.querySelector('.js-ci-variable-list-section'); + + mock = new MockAdapter(axios); + + const ajaxVariableListEl = document.querySelector('.js-ci-variable-list-section'); + saveButton = ajaxVariableListEl.querySelector('.js-secret-variables-save-button'); + errorBox = container.querySelector('.js-ci-variable-error-box'); + ajaxVariableList = new AjaxFormVariableList({ + container, + formField: 'variables', + saveButton, + errorBox, + saveEndpoint: container.dataset.saveEndpoint, + }); + + spyOn(ajaxVariableList, 'updateRowsWithPersistedVariables').and.callThrough(); + spyOn(ajaxVariableList.variableList, 'toggleEnableRow').and.callThrough(); + }); + + afterEach(() => { + mock.restore(); + }); + + describe('onSaveClicked', () => { + it('shows loading spinner while waiting for the request', (done) => { + const loadingIcon = saveButton.querySelector('.js-secret-variables-save-loading-icon'); + + mock.onPatch(VARIABLE_PATCH_ENDPOINT).reply(() => { + expect(loadingIcon.classList.contains('hide')).toEqual(false); + + return [200, {}]; + }); + + expect(loadingIcon.classList.contains('hide')).toEqual(true); + + ajaxVariableList.onSaveClicked() + .then(() => { + expect(loadingIcon.classList.contains('hide')).toEqual(true); + }) + .then(done) + .catch(done.fail); + }); + + it('calls `updateRowsWithPersistedVariables` with the persisted variables', (done) => { + const variablesResponse = [{ id: 1, key: 'foo', value: 'bar' }]; + mock.onPatch(VARIABLE_PATCH_ENDPOINT).reply(200, { + variables: variablesResponse, + }); + + ajaxVariableList.onSaveClicked() + .then(() => { + expect(ajaxVariableList.updateRowsWithPersistedVariables) + .toHaveBeenCalledWith(variablesResponse); + }) + .then(done) + .catch(done.fail); + }); + + it('hides any previous error box', (done) => { + mock.onPatch(VARIABLE_PATCH_ENDPOINT).reply(200); + + expect(errorBox.classList.contains('hide')).toEqual(true); + + ajaxVariableList.onSaveClicked() + .then(() => { + expect(errorBox.classList.contains('hide')).toEqual(true); + }) + .then(done) + .catch(done.fail); + }); + + it('disables remove buttons while waiting for the request', (done) => { + mock.onPatch(VARIABLE_PATCH_ENDPOINT).reply(() => { + expect(ajaxVariableList.variableList.toggleEnableRow).toHaveBeenCalledWith(false); + + return [200, {}]; + }); + + ajaxVariableList.onSaveClicked() + .then(() => { + expect(ajaxVariableList.variableList.toggleEnableRow).toHaveBeenCalledWith(true); + }) + .then(done) + .catch(done.fail); + }); + + it('shows error box with validation errors', (done) => { + const validationError = 'some validation error'; + mock.onPatch(VARIABLE_PATCH_ENDPOINT).reply(400, [ + validationError, + ]); + + expect(errorBox.classList.contains('hide')).toEqual(true); + + ajaxVariableList.onSaveClicked() + .then(() => { + expect(errorBox.classList.contains('hide')).toEqual(false); + expect(errorBox.textContent.trim().replace(/\n+\s+/m, ' ')).toEqual(`Validation failed ${validationError}`); + }) + .then(done) + .catch(done.fail); + }); + + it('shows flash message when request fails', (done) => { + mock.onPatch(VARIABLE_PATCH_ENDPOINT).reply(500); + + expect(errorBox.classList.contains('hide')).toEqual(true); + + ajaxVariableList.onSaveClicked() + .then(() => { + expect(errorBox.classList.contains('hide')).toEqual(true); + }) + .then(done) + .catch(done.fail); + }); + }); + + describe('updateRowsWithPersistedVariables', () => { + beforeEach(() => { + loadFixtures('projects/ci_cd_settings_with_variables.html.raw'); + container = document.querySelector('.js-ci-variable-list-section'); + + const ajaxVariableListEl = document.querySelector('.js-ci-variable-list-section'); + saveButton = ajaxVariableListEl.querySelector('.js-secret-variables-save-button'); + errorBox = container.querySelector('.js-ci-variable-error-box'); + ajaxVariableList = new AjaxFormVariableList({ + container, + formField: 'variables', + saveButton, + errorBox, + saveEndpoint: container.dataset.saveEndpoint, + }); + }); + + it('removes variable that was removed', () => { + expect(container.querySelectorAll('.js-row').length).toBe(3); + + container.querySelector('.js-row-remove-button').click(); + + expect(container.querySelectorAll('.js-row').length).toBe(3); + + ajaxVariableList.updateRowsWithPersistedVariables([]); + + expect(container.querySelectorAll('.js-row').length).toBe(2); + }); + + it('updates new variable row with persisted ID', () => { + const row = container.querySelector('.js-row:last-child'); + const idInput = row.querySelector('.js-ci-variable-input-id'); + const keyInput = row.querySelector('.js-ci-variable-input-key'); + const valueInput = row.querySelector('.js-ci-variable-input-value'); + + keyInput.value = 'foo'; + keyInput.dispatchEvent(new Event('input')); + valueInput.value = 'bar'; + valueInput.dispatchEvent(new Event('input')); + + expect(idInput.value).toEqual(''); + + ajaxVariableList.updateRowsWithPersistedVariables([{ + id: 3, + key: 'foo', + value: 'bar', + }]); + + expect(idInput.value).toEqual('3'); + expect(row.dataset.isPersisted).toEqual('true'); + }); + }); +}); diff --git a/spec/javascripts/ci_variable_list/ci_variable_list_spec.js b/spec/javascripts/ci_variable_list/ci_variable_list_spec.js index 0170ab458d4..6ab7b50e035 100644 --- a/spec/javascripts/ci_variable_list/ci_variable_list_spec.js +++ b/spec/javascripts/ci_variable_list/ci_variable_list_spec.js @@ -4,6 +4,7 @@ import getSetTimeoutPromise from '../helpers/set_timeout_promise_helper'; describe('VariableList', () => { preloadFixtures('pipeline_schedules/edit.html.raw'); preloadFixtures('pipeline_schedules/edit_with_variables.html.raw'); + preloadFixtures('projects/ci_cd_settings.html.raw'); let $wrapper; let variableList; @@ -105,37 +106,8 @@ describe('VariableList', () => { describe('with all inputs(key, value, protected)', () => { beforeEach(() => { - // This markup will be replaced with a fixture when we can render the - // CI/CD settings page with the new dynamic variable list in https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/4110 - $wrapper = $(`<form class="js-variable-list"> - <ul> - <li class="js-row"> - <div class="ci-variable-body-item"> - <input class="js-ci-variable-input-key" name="variables[variables_attributes][][key]"> - </div> - - <div class="ci-variable-body-item"> - <textarea class="js-ci-variable-input-value" name="variables[variables_attributes][][value]"></textarea> - </div> - - <div class="ci-variable-body-item ci-variable-protected-item"> - <button type="button" class="js-project-feature-toggle project-feature-toggle"> - <input - type="hidden" - class="js-ci-variable-input-protected js-project-feature-toggle-input" - name="variables[variables_attributes][][protected]" - value="true" - /> - </button> - </div> - - <button type="button" class="js-row-remove-button"></button> - </li> - </ul> - <button type="button" class="js-secret-value-reveal-button"> - Reveal values - </button> - </form>`); + loadFixtures('projects/ci_cd_settings.html.raw'); + $wrapper = $('.js-ci-variable-list-section'); variableList = new VariableList({ container: $wrapper, @@ -160,4 +132,51 @@ describe('VariableList', () => { .catch(done.fail); }); }); + + describe('toggleEnableRow method', () => { + beforeEach(() => { + loadFixtures('pipeline_schedules/edit_with_variables.html.raw'); + $wrapper = $('.js-ci-variable-list-section'); + + variableList = new VariableList({ + container: $wrapper, + formField: 'variables', + }); + variableList.init(); + }); + + it('should disable all key inputs', () => { + expect($wrapper.find('.js-ci-variable-input-key:not([disabled])').length).toBe(3); + + variableList.toggleEnableRow(false); + + expect($wrapper.find('.js-ci-variable-input-key[disabled]').length).toBe(3); + }); + + it('should disable all remove buttons', () => { + expect($wrapper.find('.js-row-remove-button:not([disabled])').length).toBe(3); + + variableList.toggleEnableRow(false); + + expect($wrapper.find('.js-row-remove-button[disabled]').length).toBe(3); + }); + + it('should enable all remove buttons', () => { + variableList.toggleEnableRow(false); + expect($wrapper.find('.js-row-remove-button[disabled]').length).toBe(3); + + variableList.toggleEnableRow(true); + + expect($wrapper.find('.js-row-remove-button:not([disabled])').length).toBe(3); + }); + + it('should enable all key inputs', () => { + variableList.toggleEnableRow(false); + expect($wrapper.find('.js-ci-variable-input-key[disabled]').length).toBe(3); + + variableList.toggleEnableRow(true); + + expect($wrapper.find('.js-ci-variable-input-key:not([disabled])').length).toBe(3); + }); + }); }); diff --git a/spec/javascripts/fixtures/groups.rb b/spec/javascripts/fixtures/groups.rb new file mode 100644 index 00000000000..35be52fbf97 --- /dev/null +++ b/spec/javascripts/fixtures/groups.rb @@ -0,0 +1,29 @@ +require 'spec_helper' + +describe 'Groups (JavaScript fixtures)', type: :controller do + include JavaScriptFixturesHelpers + + let(:admin) { create(:admin) } + let(:group) { create(:group, name: 'frontend-fixtures-group' )} + + render_views + + before(:all) do + clean_frontend_fixtures('groups/') + end + + before do + group.add_master(admin) + sign_in(admin) + end + + describe Groups::Settings::CiCdController, '(JavaScript fixtures)', type: :controller do + it 'groups/ci_cd_settings.html.raw' do |example| + get :show, + group_id: group + + expect(response).to be_success + store_frontend_fixture(response, example.description) + end + end +end diff --git a/spec/javascripts/fixtures/projects.rb b/spec/javascripts/fixtures/projects.rb index 2a100e7fab5..b344b389241 100644 --- a/spec/javascripts/fixtures/projects.rb +++ b/spec/javascripts/fixtures/projects.rb @@ -1,11 +1,14 @@ require 'spec_helper' -describe ProjectsController, '(JavaScript fixtures)', type: :controller do +describe 'Projects (JavaScript fixtures)', type: :controller do include JavaScriptFixturesHelpers let(:admin) { create(:admin) } let(:namespace) { create(:namespace, name: 'frontend-fixtures' )} let(:project) { create(:project, namespace: namespace, path: 'builds-project') } + let(:project_variable_populated) { create(:project, namespace: namespace, path: 'builds-project2') } + let!(:variable1) { create(:ci_variable, project: project_variable_populated) } + let!(:variable2) { create(:ci_variable, project: project_variable_populated) } render_views @@ -14,6 +17,9 @@ describe ProjectsController, '(JavaScript fixtures)', type: :controller do end before do + # EE-specific start + # EE specific end + project.add_master(admin) sign_in(admin) end @@ -21,12 +27,43 @@ describe ProjectsController, '(JavaScript fixtures)', type: :controller do remove_repository(project) end - it 'projects/dashboard.html.raw' do |example| - get :show, - namespace_id: project.namespace.to_param, - id: project + describe ProjectsController, '(JavaScript fixtures)', type: :controller do + it 'projects/dashboard.html.raw' do |example| + get :show, + namespace_id: project.namespace.to_param, + id: project - expect(response).to be_success - store_frontend_fixture(response, example.description) + expect(response).to be_success + store_frontend_fixture(response, example.description) + end + + it 'projects/edit.html.raw' do |example| + get :edit, + namespace_id: project.namespace.to_param, + id: project + + expect(response).to be_success + store_frontend_fixture(response, example.description) + end + end + + describe Projects::Settings::CiCdController, '(JavaScript fixtures)', type: :controller do + it 'projects/ci_cd_settings.html.raw' do |example| + get :show, + namespace_id: project.namespace.to_param, + project_id: project + + expect(response).to be_success + store_frontend_fixture(response, example.description) + end + + it 'projects/ci_cd_settings_with_variables.html.raw' do |example| + get :show, + namespace_id: project_variable_populated.namespace.to_param, + project_id: project_variable_populated + + expect(response).to be_success + store_frontend_fixture(response, example.description) + end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index a63f5d6d5a1..97bb77766fb 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2070,7 +2070,7 @@ describe Project do create(:ci_variable, :protected, value: 'protected', project: project) end - subject { project.secret_variables_for(ref: 'ref') } + subject { project.reload.secret_variables_for(ref: 'ref') } before do stub_application_setting( diff --git a/spec/presenters/ci/group_variable_presenter_spec.rb b/spec/presenters/ci/group_variable_presenter_spec.rb index d404028405b..cb58a757564 100644 --- a/spec/presenters/ci/group_variable_presenter_spec.rb +++ b/spec/presenters/ci/group_variable_presenter_spec.rb @@ -35,29 +35,20 @@ describe Ci::GroupVariablePresenter do end describe '#form_path' do - context 'when variable is persisted' do - subject { described_class.new(variable).form_path } + subject { described_class.new(variable).form_path } - it { is_expected.to eq(group_variable_path(group, variable)) } - end - - context 'when variable is not persisted' do - let(:variable) { build(:ci_group_variable, group: group) } - subject { described_class.new(variable).form_path } - - it { is_expected.to eq(group_variables_path(group)) } - end + it { is_expected.to eq(group_settings_ci_cd_path(group)) } end describe '#edit_path' do subject { described_class.new(variable).edit_path } - it { is_expected.to eq(group_variable_path(group, variable)) } + it { is_expected.to eq(group_variables_path(group)) } end describe '#delete_path' do subject { described_class.new(variable).delete_path } - it { is_expected.to eq(group_variable_path(group, variable)) } + it { is_expected.to eq(group_variables_path(group)) } end end diff --git a/spec/presenters/ci/variable_presenter_spec.rb b/spec/presenters/ci/variable_presenter_spec.rb index db62f86edb0..e3ce88372ea 100644 --- a/spec/presenters/ci/variable_presenter_spec.rb +++ b/spec/presenters/ci/variable_presenter_spec.rb @@ -35,29 +35,20 @@ describe Ci::VariablePresenter do end describe '#form_path' do - context 'when variable is persisted' do - subject { described_class.new(variable).form_path } + subject { described_class.new(variable).form_path } - it { is_expected.to eq(project_variable_path(project, variable)) } - end - - context 'when variable is not persisted' do - let(:variable) { build(:ci_variable, project: project) } - subject { described_class.new(variable).form_path } - - it { is_expected.to eq(project_variables_path(project)) } - end + it { is_expected.to eq(project_settings_ci_cd_path(project)) } end describe '#edit_path' do subject { described_class.new(variable).edit_path } - it { is_expected.to eq(project_variable_path(project, variable)) } + it { is_expected.to eq(project_variables_path(project)) } end describe '#delete_path' do subject { described_class.new(variable).delete_path } - it { is_expected.to eq(project_variable_path(project, variable)) } + it { is_expected.to eq(project_variables_path(project)) } end end diff --git a/spec/requests/api/group_variables_spec.rb b/spec/requests/api/group_variables_spec.rb index a4f198eb5c9..64fa7dc824c 100644 --- a/spec/requests/api/group_variables_spec.rb +++ b/spec/requests/api/group_variables_spec.rb @@ -142,12 +142,12 @@ describe API::GroupVariables do end it 'updates variable data' do - initial_variable = group.variables.first + initial_variable = group.variables.reload.first value_before = initial_variable.value put api("/groups/#{group.id}/variables/#{variable.key}", user), value: 'VALUE_1_UP', protected: true - updated_variable = group.variables.first + updated_variable = group.variables.reload.first expect(response).to have_gitlab_http_status(200) expect(value_before).to eq(variable.value) diff --git a/spec/requests/api/variables_spec.rb b/spec/requests/api/variables_spec.rb index 79ee6c126f6..62215ea3d7d 100644 --- a/spec/requests/api/variables_spec.rb +++ b/spec/requests/api/variables_spec.rb @@ -122,12 +122,12 @@ describe API::Variables do describe 'PUT /projects/:id/variables/:key' do context 'authorized user with proper permissions' do it 'updates variable data' do - initial_variable = project.variables.first + initial_variable = project.variables.reload.first value_before = initial_variable.value put api("/projects/#{project.id}/variables/#{variable.key}", user), value: 'VALUE_1_UP', protected: true - updated_variable = project.variables.first + updated_variable = project.variables.reload.first expect(response).to have_gitlab_http_status(200) expect(value_before).to eq(variable.value) diff --git a/spec/serializers/group_variable_entity_spec.rb b/spec/serializers/group_variable_entity_spec.rb new file mode 100644 index 00000000000..f6de7d01f98 --- /dev/null +++ b/spec/serializers/group_variable_entity_spec.rb @@ -0,0 +1,14 @@ +require 'spec_helper' + +describe GroupVariableEntity do + let(:variable) { create(:ci_group_variable) } + let(:entity) { described_class.new(variable) } + + describe '#as_json' do + subject { entity.as_json } + + it 'contains required fields' do + expect(subject).to include(:id, :key, :value, :protected) + end + end +end diff --git a/spec/serializers/variable_entity_spec.rb b/spec/serializers/variable_entity_spec.rb new file mode 100644 index 00000000000..effc0022633 --- /dev/null +++ b/spec/serializers/variable_entity_spec.rb @@ -0,0 +1,14 @@ +require 'spec_helper' + +describe VariableEntity do + let(:variable) { create(:ci_variable) } + let(:entity) { described_class.new(variable) } + + describe '#as_json' do + subject { entity.as_json } + + it 'contains required fields' do + expect(subject).to include(:id, :key, :value, :protected) + end + end +end diff --git a/spec/support/features/variable_list_shared_examples.rb b/spec/support/features/variable_list_shared_examples.rb new file mode 100644 index 00000000000..83bf06b6727 --- /dev/null +++ b/spec/support/features/variable_list_shared_examples.rb @@ -0,0 +1,269 @@ +shared_examples 'variable list' do + it 'shows list of variables' do + page.within('.js-ci-variable-list-section') do + expect(first('.js-ci-variable-input-key').value).to eq(variable.key) + end + end + + it 'adds new secret variable' do + page.within('.js-ci-variable-list-section .js-row:last-child') do + find('.js-ci-variable-input-key').set('key') + find('.js-ci-variable-input-value').set('key value') + end + + click_button('Save variables') + wait_for_requests + + visit page_path + + # We check the first row because it re-sorts to alphabetical order on refresh + page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do + expect(find('.js-ci-variable-input-key').value).to eq('key') + expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key value') + end + end + + it 'adds empty variable' do + page.within('.js-ci-variable-list-section .js-row:last-child') do + find('.js-ci-variable-input-key').set('key') + find('.js-ci-variable-input-value').set('') + end + + click_button('Save variables') + wait_for_requests + + visit page_path + + # We check the first row because it re-sorts to alphabetical order on refresh + page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do + expect(find('.js-ci-variable-input-key').value).to eq('key') + expect(find('.js-ci-variable-input-value', visible: false).value).to eq('') + end + end + + it 'adds new unprotected variable' do + page.within('.js-ci-variable-list-section .js-row:last-child') do + find('.js-ci-variable-input-key').set('key') + find('.js-ci-variable-input-value').set('key value') + find('.ci-variable-protected-item .js-project-feature-toggle').click + + expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('false') + end + + click_button('Save variables') + wait_for_requests + + visit page_path + + # We check the first row because it re-sorts to alphabetical order on refresh + page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do + expect(find('.js-ci-variable-input-key').value).to eq('key') + expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key value') + expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('false') + end + end + + it 'reveals and hides variables' do + page.within('.js-ci-variable-list-section') do + expect(first('.js-ci-variable-input-key').value).to eq(variable.key) + expect(first('.js-ci-variable-input-value', visible: false).value).to eq(variable.value) + expect(page).to have_content('*' * 20) + + click_button('Reveal value') + + expect(first('.js-ci-variable-input-key').value).to eq(variable.key) + expect(first('.js-ci-variable-input-value').value).to eq(variable.value) + expect(page).not_to have_content('*' * 20) + + click_button('Hide value') + + expect(first('.js-ci-variable-input-key').value).to eq(variable.key) + expect(first('.js-ci-variable-input-value', visible: false).value).to eq(variable.value) + expect(page).to have_content('*' * 20) + end + end + + it 'deletes variable' do + page.within('.js-ci-variable-list-section') do + expect(page).to have_selector('.js-row', count: 2) + + first('.js-row-remove-button').click + + click_button('Save variables') + wait_for_requests + + expect(page).to have_selector('.js-row', count: 1) + end + end + + it 'edits variable' do + page.within('.js-ci-variable-list-section') do + click_button('Reveal value') + + page.within('.js-row:nth-child(1)') do + find('.js-ci-variable-input-key').set('new_key') + find('.js-ci-variable-input-value').set('new_value') + end + + click_button('Save variables') + wait_for_requests + + visit page_path + + page.within('.js-row:nth-child(1)') do + expect(find('.js-ci-variable-input-key').value).to eq('new_key') + expect(find('.js-ci-variable-input-value', visible: false).value).to eq('new_value') + end + end + end + + it 'edits variable with empty value' do + page.within('.js-ci-variable-list-section') do + click_button('Reveal value') + + page.within('.js-row:nth-child(1)') do + find('.js-ci-variable-input-key').set('new_key') + find('.js-ci-variable-input-value').set('') + end + + click_button('Save variables') + wait_for_requests + + visit page_path + + page.within('.js-row:nth-child(1)') do + expect(find('.js-ci-variable-input-key').value).to eq('new_key') + expect(find('.js-ci-variable-input-value', visible: false).value).to eq('') + end + end + end + + it 'edits variable to be protected' do + # Create the unprotected variable + page.within('.js-ci-variable-list-section .js-row:last-child') do + find('.js-ci-variable-input-key').set('unprotected_key') + find('.js-ci-variable-input-value').set('unprotected_value') + find('.ci-variable-protected-item .js-project-feature-toggle').click + + expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('false') + end + + click_button('Save variables') + wait_for_requests + + visit page_path + + # We check the first row because it re-sorts to alphabetical order on refresh + page.within('.js-ci-variable-list-section .js-row:nth-child(2)') do + find('.ci-variable-protected-item .js-project-feature-toggle').click + + expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') + end + + click_button('Save variables') + wait_for_requests + + visit page_path + + # We check the first row because it re-sorts to alphabetical order on refresh + page.within('.js-ci-variable-list-section .js-row:nth-child(2)') do + expect(find('.js-ci-variable-input-key').value).to eq('unprotected_key') + expect(find('.js-ci-variable-input-value', visible: false).value).to eq('unprotected_value') + expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') + end + end + + it 'edits variable to be unprotected' do + # Create the protected variable + page.within('.js-ci-variable-list-section .js-row:last-child') do + find('.js-ci-variable-input-key').set('protected_key') + find('.js-ci-variable-input-value').set('protected_value') + + expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') + end + + click_button('Save variables') + wait_for_requests + + visit page_path + + page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do + find('.ci-variable-protected-item .js-project-feature-toggle').click + + expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('false') + end + + click_button('Save variables') + wait_for_requests + + visit page_path + + page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do + expect(find('.js-ci-variable-input-key').value).to eq('protected_key') + expect(find('.js-ci-variable-input-value', visible: false).value).to eq('protected_value') + expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('false') + end + end + + it 'handles multiple edits and deletion in the middle' do + page.within('.js-ci-variable-list-section') do + # Create 2 variables + page.within('.js-row:last-child') do + find('.js-ci-variable-input-key').set('akey') + find('.js-ci-variable-input-value').set('akeyvalue') + end + page.within('.js-row:last-child') do + find('.js-ci-variable-input-key').set('zkey') + find('.js-ci-variable-input-value').set('zkeyvalue') + end + + click_button('Save variables') + wait_for_requests + + expect(page).to have_selector('.js-row', count: 4) + + # Remove the `akey` variable + page.within('.js-row:nth-child(2)') do + first('.js-row-remove-button').click + end + + # Add another variable + page.within('.js-row:last-child') do + find('.js-ci-variable-input-key').set('ckey') + find('.js-ci-variable-input-value').set('ckeyvalue') + end + + click_button('Save variables') + wait_for_requests + + visit page_path + + # Expect to find 3 variables(4 rows) in alphbetical order + expect(page).to have_selector('.js-row', count: 4) + row_keys = all('.js-ci-variable-input-key') + expect(row_keys[0].value).to eq('ckey') + expect(row_keys[1].value).to eq('test_key') + expect(row_keys[2].value).to eq('zkey') + expect(row_keys[3].value).to eq('') + end + end + + it 'shows validation error box about duplicate keys' do + page.within('.js-ci-variable-list-section .js-row:last-child') do + find('.js-ci-variable-input-key').set('samekey') + find('.js-ci-variable-input-value').set('value1') + end + page.within('.js-ci-variable-list-section .js-row:last-child') do + find('.js-ci-variable-input-key').set('samekey') + find('.js-ci-variable-input-value').set('value2') + end + + click_button('Save variables') + wait_for_requests + + # We check the first row because it re-sorts to alphabetical order on refresh + page.within('.js-ci-variable-list-section') do + expect(find('.js-ci-variable-error-box')).to have_content('Validation failed Variables Duplicate variables: samekey') + end + end +end diff --git a/spec/support/shared_examples/controllers/variables_shared_examples.rb b/spec/support/shared_examples/controllers/variables_shared_examples.rb new file mode 100644 index 00000000000..d7acf8c0032 --- /dev/null +++ b/spec/support/shared_examples/controllers/variables_shared_examples.rb @@ -0,0 +1,123 @@ +shared_examples 'GET #show lists all variables' do + it 'renders the variables as json' do + subject + + expect(response).to match_response_schema('variables') + end + + it 'has only one variable' do + subject + + expect(json_response['variables'].count).to eq(1) + end +end + +shared_examples 'PATCH #update updates variables' do + let(:variable_attributes) do + { id: variable.id, + key: variable.key, + value: variable.value, + protected: variable.protected?.to_s } + end + let(:new_variable_attributes) do + { key: 'new_key', + value: 'dummy_value', + protected: 'false' } + end + + context 'with invalid new variable parameters' do + let(:variables_attributes) do + [ + variable_attributes.merge(value: 'other_value'), + new_variable_attributes.merge(key: '...?') + ] + end + + it 'does not update the existing variable' do + expect { subject }.not_to change { variable.reload.value } + end + + it 'does not create the new variable' do + expect { subject }.not_to change { owner.variables.count } + end + + it 'returns a bad request response' do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + end + end + + context 'with duplicate new variable parameters' do + let(:variables_attributes) do + [ + new_variable_attributes, + new_variable_attributes.merge(value: 'other_value') + ] + end + + it 'does not update the existing variable' do + expect { subject }.not_to change { variable.reload.value } + end + + it 'does not create the new variable' do + expect { subject }.not_to change { owner.variables.count } + end + + it 'returns a bad request response' do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + end + end + + context 'with valid new variable parameters' do + let(:variables_attributes) do + [ + variable_attributes.merge(value: 'other_value'), + new_variable_attributes + ] + end + + it 'updates the existing variable' do + expect { subject }.to change { variable.reload.value }.to('other_value') + end + + it 'creates the new variable' do + expect { subject }.to change { owner.variables.count }.by(1) + end + + it 'returns a successful response' do + subject + + expect(response).to have_gitlab_http_status(:ok) + end + + it 'has all variables in response' do + subject + + expect(response).to match_response_schema('variables') + end + end + + context 'with a deleted variable' do + let(:variables_attributes) { [variable_attributes.merge(_destroy: 'true')] } + + it 'destroys the variable' do + expect { subject }.to change { owner.variables.count }.by(-1) + expect { variable.reload }.to raise_error ActiveRecord::RecordNotFound + end + + it 'returns a successful response' do + subject + + expect(response).to have_gitlab_http_status(:ok) + end + + it 'has all variables in response' do + subject + + expect(response).to match_response_schema('variables') + end + end +end |
