Merge branch '39118-dynamic-pipeline-variables-fe' into 'master'

Dynamic CI secret variables

Closes gitlab-ce#39118

See merge request gitlab-org/gitlab-ee!4110

app/assets/javascripts/ci_variable_list/ajax_variable_list.js

+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');
+        }
+      }
+    });
+  }
+}

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,

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]);

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,
 };

app/assets/javascripts/pages/groups/settings/ci_cd/show/index.js

-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,
+  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,
   });
-    secretVariableTableValues.init();
-  }
 };

app/assets/javascripts/pages/projects/settings/ci_cd/show/index.js

 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,
+  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,
   });
-    secretVariableTableValues.init();
-  }
 }

app/assets/stylesheets/framework/ci_variable_list.scss

@@ -8,7 +8,11 @@
 
 .ci-variable-row {
   display: flex;
+  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;
+  }
 }

app/controllers/groups/variables_controller.rb

 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
-
-    def update
-      if variable.update(variable_params)
-        redirect_to group_variables_path(group),
-                    notice: 'Variable was successfully updated.'
-      else
-        render "show"
       end
     end
 
-    def create
-      @variable = group.variables.create(variable_params)
-        .present(current_user: current_user)
-
-      if @variable.persisted?
-        redirect_to group_settings_ci_cd_path(group),
-                    notice: 'Variable was successfully created.'
-      else
-        render "show"
-      end
+    def update
+      if @group.update(group_variables_params)
+        respond_to do |format|
+          format.json { return render_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.'
+        respond_to do |format|
+          format.json { render_error }
+        end
       end
     end
 
     private
 
-    def variable_params
-      params.require(:variable).permit(*variable_params_attributes)
+    def render_group_variables
+      render status: :ok, json: { variables: GroupVariableSerializer.new.represent(@group.variables) }
     end
 
-    def variable_params_attributes
-      %i[key value protected]
+    def render_error
+      render status: :bad_request, json: @group.errors.full_messages
     end
 
-    def variable
-      @variable ||= group.variables.find(params[:id]).present(current_user: current_user)
+    def group_variables_params
+      params.permit(variables_attributes: [*variable_params_attributes])
+    end
+
+    def variable_params_attributes
+      %i[id key value protected _destroy]
     end
 
     def authorize_admin_build!

app/controllers/projects/variables_controller.rb

 class Projects::VariablesController < Projects::ApplicationController
   prepend ::EE::Projects::VariablesController
 
-  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

app/models/group.rb

@@ -40,9 +40,12 @@ class Group < Namespace
   # here since Group inherits from Namespace, the entity_type would be set to `Namespace`.
   has_many :audit_events, -> { where(entity_type: Group) }, foreign_key: 'entity_id'
 
+  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 }
 

app/models/project.rb

@@ -266,6 +266,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
 

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

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

app/serializers/group_variable_entity.rb

+class GroupVariableEntity < Grape::Entity
+  expose :id
+  expose :key
+  expose :value
+
+  expose :protected?, as: :protected
+end

app/serializers/group_variable_serializer.rb

+class GroupVariableSerializer < BaseSerializer
+  entity GroupVariableEntity
+end

app/serializers/variable_entity.rb

+class VariableEntity < Grape::Entity
+  prepend ::EE::VariableEntity
+
+  expose :id
+  expose :key
+  expose :value
+
+  expose :protected?, as: :protected
+end

app/serializers/variable_serializer.rb

+class VariableSerializer < BaseSerializer
+  entity VariableEntity
+end

app/views/ci/variables/_content.html.haml

-%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.')

app/views/ci/variables/_form.html.haml

-= 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
-  - if @variable.respond_to?(:environment_scope)
-    = render 'projects/variables/ee/environment_scope', f: f
-  .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"

app/views/ci/variables/_index.html.haml

-.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.
+- 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
-      .js-secret-variable-table
-        = render "ci/variables/table"
-        %button.btn.btn-info.js-secret-value-reveal-button{ data: { secret_reveal_status: 'false' } }
           = n_('Reveal value', 'Reveal values', @variables.size)

app/views/ci/variables/_show.html.haml

-- 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"

app/views/ci/variables/_table.html.haml

-.table-responsive.variables-table
-  %table.table
-    %colgroup
-      %col
-      %col
-      %col
-      %col{ width: 100 }
-    %thead
-      %th Key
-      %th Value
-      %th Protected
-      - if @variable.respond_to?(:environment_scope) && @project.feature_available?(:variable_environment_scope)
-        %th Environment scope
-      %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)
-            - if @variable.respond_to?(:environment_scope) && @project.feature_available?(:variable_environment_scope)
-              %td.variable-environment-scope= variable.environment_scope
-            %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")

app/views/groups/settings/ci_cd/show.html.haml

 - 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

app/views/groups/variables/show.html.haml

-= render 'ci/variables/show'

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

app/views/projects/variables/show.html.haml

-= render 'ci/variables/show'

changelogs/unreleased-ee/39118-dynamic-pipeline-variables-fe.yml

+---
+title: Update CI/CD secret variables list to be dynamic and save without reloading
+  the page
+merge_request: 4110
+author:
+type: added

config/routes/group.rb

@@ -29,7 +29,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]
 

config/routes/project.rb

@@ -181,7 +181,8 @@ constraints(ProjectUrlConstrainer.new) do
       get '/service_desk' => 'service_desk#show', as: :service_desk
       put '/service_desk' => 'service_desk#update', as: :service_desk_refresh
 
-      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

ee/app/serializers/ee/variable_entity.rb

+module EE
+  module VariableEntity
+    extend ActiveSupport::Concern
+
+    prepended do
+      expose :environment_scope, if: ->(variable, options) { variable.project.feature_available?(:variable_environment_scope) }
+    end
+  end
+end

ee/app/views/ci/variables/_environment_scope.html.haml

@@ -5,7 +5,7 @@
   - environment_scope = variable&.environment_scope || '*'
   - environment_scope_label = environment_scope == '*' ? s_('CiVariable|All environments') : environment_scope
 
-  %input{ type: "hidden", name: "#{form_field}[variables_attributes][][environment]", value: environment_scope }
+  %input{ type: "hidden", name: "#{form_field}[variables_attributes][][environment_scope]", value: environment_scope }
   = dropdown_tag(environment_scope_label,
                  options: { wrapper_class: 'ci-variable-body-item js-variable-environment-dropdown-wrapper',
                             toggle_class: 'js-variable-environment-toggle wide',
@@ -17,5 +17,5 @@
     %ul.dropdown-footer-list
       %li
         %button{ class: "dropdown-create-new-item-button js-dropdown-create-new-item", title: s_('CiVariable|New environment') }
-          - s_('CiVariable|Create wildcard')
+          = s_('CiVariable|Create wildcard')
           %code

ee/app/views/projects/variables/ee/_environment_scope.html.haml

-- if @project.feature_available?(:variable_environment_scope)
-  .form-group
-    = f.label :environment_scope, "Environment scope", class: "label-light"
-    = f.text_field :environment_scope, class: "form-control", placeholder: "*"
-    .help-block
-      This variable will be passed only to jobs with a matching environment name.
-      <code>*</code> is a wildcard that matches all environments (existing or not).
-      = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'limiting-environment-scopes-of-secret-variables'), target: '_blank'

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

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
-
-          def add_variable
-            click_on 'Add new variable'
           end
 
-          def variable_key
-            page.find('.variable-key').text
+          def save_variables
+            click_button('Save variables')
           end
 
-          def variable_value
-            reveal_value do
-              page.find('.variable-value').text
+          def variable_key
+            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

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" }
+  describe 'GET #show' do
+    let!(:variable) { create(:ci_group_variable, group: group) }
 
-        expect(flash[:notice]).to include 'Variable was successfully created.'
-        expect(response).to redirect_to(group_settings_ci_cd_path(group))
+    subject do
+      get :show, group_id: group, format: :json
     end
-    end
-
-    context 'variable is invalid' do
-      it 'renders show' do
-        post :create, group_id: group, variable: { key: "..one", value: "two" }
 
-        expect(response).to render_template("groups/variables/show")
-      end
-    end
+    include_examples 'GET #show lists all variables'
   end
 
-  describe 'POST #update' do
-    let(:variable) { create(:ci_group_variable) }
+  describe 'PATCH #update' do
+    let!(:variable) { create(:ci_group_variable, group: group) }
+    let(:owner) { group }
 
-    context 'updating a variable with valid characters' do
-      before do
-        group.variables << variable
+    subject do
+      patch :update,
+        group_id: group,
+        variables_attributes: variables_attributes,
+        format: :json
     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
-
-      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 }
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(response).to render_template :show
-      end
-    end
+    include_examples 'PATCH #update updates variables'
   end
 end

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
-    end
+    subject do
+      get :show, namespace_id: project.namespace.to_param, project_id: project, format: :json
     end
 
-  describe 'POST #update' do
-    let(:variable) { create(:ci_variable) }
-
-    context 'updating a variable with valid characters' do
-      before do
-        project.variables << variable
+    include_examples 'GET #show lists all variables'
   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' }
+  describe 'PATCH #update' do
+    let!(:variable) { create(:ci_variable, project: project) }
+    let(:owner) { project }
 
-        expect(flash[:notice]).to include 'Variable was successfully updated.'
-        expect(response).to redirect_to(project_variables_path(project))
+    subject do
+      patch :update,
+        namespace_id: project.namespace.to_param,
+        project_id: project,
+        variables_attributes: variables_attributes,
+        format: :json
     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 }
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(response).to render_template :show
-      end
-    end
+    include_examples 'PATCH #update updates variables'
   end
 end

spec/ee/spec/features/project_variables_spec.rb

+require 'spec_helper'
+
+describe 'Project variables EE', :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
+    stub_licensed_features(variable_environment_scope: variable_environment_scope)
+
+    login_as(user)
+    project.add_master(user)
+    project.variables << variable
+
+    visit page_path
+  end
+
+  context 'when variable environment scope is available' do
+    let(:variable_environment_scope) { true }
+
+    it 'adds new variable with a special environment scope' do
+      page.within('.js-ci-variable-list-section .js-row:last-child') do
+        find('.js-ci-variable-input-key').set('somekey')
+        find('.js-ci-variable-input-value').set('somevalue')
+
+        find('.js-variable-environment-toggle').click
+        find('.js-variable-environment-dropdown-wrapper .dropdown-input-field').set('review/*')
+        find('.js-variable-environment-dropdown-wrapper .js-dropdown-create-new-item').click
+
+        expect(find('input[name="variables[variables_attributes][][environment_scope]"]', visible: false).value).to eq('review/*')
+      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('somekey')
+        expect(page).to have_content('review/*')
+      end
+    end
+  end
+
+  context 'when variable environment scope is not available' do
+    let(:variable_environment_scope) { false }
+
+    it 'does not show variable environment scope element' do
+      expect(page).not_to have_selector('input[name="variables[variables_attributes][][environment_scope]"]')
+      expect(page).not_to have_selector('.js-variable-environment-dropdown-wrapper')
+    end
+  end
+end

spec/ee/spec/features/variables_spec.rb

-require 'spec_helper'
-
-describe 'Project variables EE', :js do
-  let(:user)     { create(:user) }
-  let(:project)  { create(:project) }
-  let(:variable) { create(:ci_variable, key: 'test_key', value: 'test value') }
-
-  let(:variable_environment_scope) { true }
-
-  before do
-    stub_licensed_features(
-      variable_environment_scope: variable_environment_scope)
-
-    login_as(user)
-    project.add_master(user)
-    project.variables << variable
-
-    visit project_settings_ci_cd_path(project)
-  end
-
-  it 'adds new variable with a special environment scope' do
-    expect(page).to have_selector('#variable_environment_scope')
-
-    fill_in('variable_key', with: 'key')
-    fill_in('variable_value', with: 'value')
-    fill_in('variable_environment_scope', with: 'review/*')
-    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('review/*')
-    end
-  end
-
-  context 'when variable environment scope is not available' do
-    let(:variable_environment_scope) { false }
-
-    it 'does not show variable environment scope element' do
-      expect(page).not_to have_selector('#variable_environment_scope')
-    end
-  end
-
-  context 'when editing a variable for environment' do
-    before do
-      page.within('.variables-table') do
-        find('.btn-variable-edit').click
-      end
-    end
-
-    it 'edits variable to be another environment scope' do
-      expect(page).to have_selector('#variable_environment_scope')
-
-      fill_in('variable_environment_scope', with: 'review/*')
-      click_button('Save variable')
-
-      expect(page).to have_content('Variable was successfully updated.')
-      expect(project.variables(true).first.environment_scope).to eq('review/*')
-    end
-
-    context 'when variable environment scope is not available' do
-      let(:variable_environment_scope) { false }
-
-      it 'does not show environment scope element' do
-        expect(page).not_to have_selector('#variable_environment_scope')
-      end
-    end
-  end
-end

spec/ee/spec/serializers/ee/variable_entity_spec.rb

+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 }
+
+    context 'when project has variable environment scopes available' do
+      before do
+        allow(variable.project).to receive(:feature_available?).with(:variable_environment_scope).and_return(true)
+      end
+
+      it 'contains the environment_scope field' do
+        expect(subject).to include(:environment_scope)
+      end
+    end
+
+    context 'when project does not have variable environment scopes available' do
+      before do
+        allow(variable.project).to receive(:feature_available?).with(:variable_environment_scope).and_return(false)
+      end
+
+      it 'does not contain the environment_scope field' do
+        expect(subject).not_to include(:environment_scope)
+      end
+    end
+  end
+end

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'
+    visit page_path
   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
-  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

spec/features/project_variables_spec.rb

+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

spec/features/variables_spec.rb

-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

spec/fixtures/api/schemas/variable.json

+{
+  "type": "object",
+  "required": [
+    "id",
+    "key",
+    "value",
+    "protected"
+  ],
+  "properties": {
+    "id": { "type": "integer" },
+    "key": { "type": "string" },
+    "value": { "type": "string" },
+    "protected": { "type": "boolean" }
+  },
+  "additionalProperties": false
+}

spec/fixtures/api/schemas/variables.json

+{
+  "type": "object",
+  "required": ["variables"],
+  "properties": {
+    "variables": {
+      "type": "array",
+      "items": { "$ref": "variable.json" }
+    }
+  },
+  "additionalProperties": false
+}

spec/javascripts/ci_variable_list/ajax_variable_list_spec.js

+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');
+    });
+  });
+});

spec/javascripts/ci_variable_list/ci_variable_list_ee_spec.js

@@ -2,74 +2,15 @@ import VariableList from '~/ci_variable_list/ci_variable_list';
 import getSetTimeoutPromise from '../helpers/set_timeout_promise_helper';
 
 describe('VariableList (EE features)', () => {
+  preloadFixtures('projects/ci_cd_settings.html.raw');
+
   let $wrapper;
   let variableList;
 
   describe('with all inputs(key, value, protected, environment)', () => {
     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>
-
-            <div class="ci-variable-body-item">
-              <input type="hidden" name="variables[variables_attributes][][environment]" value="*" />
-              <div class="dropdown js-variable-environment-dropdown-wrapper">
-                <button
-                  type="button"
-                  class="dropdown-menu-toggle js-variable-environment-toggle"
-                  data-toggle="dropdown"
-                >
-                  <span class="dropdown-toggle-text js-dropdown-toggle-text"></span>
-                </button>
-                <div class="dropdown-menu dropdown-select dropdown-menu-selectable">
-                  <div class="dropdown-input has-value">
-                    <input type="search" class="dropdown-input-field js-dropdown-input-field">
-                    <button class="dropdown-input-clear js-dropdown-input-clear">
-                      Clear search
-                    </button>
-                  </div>
-                  <div class="dropdown-content js-dropdown-content"></div>
-                  <div class="dropdown-footer">
-                    <ul class="dropdown-footer-list">
-                      <li>
-                        <button class="dropdown-create-new-item-button js-dropdown-create-new-item">
-                          Create wildcard
-                          <code class="js-drodpown-create-new-item-slot"></code>
-                        </button>
-                      </li>
-                    </ul>
-                  </div>
-                </div>
-              </div>
-            </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,
@@ -102,12 +43,38 @@ describe('VariableList (EE features)', () => {
             expect($wrapper.find('.js-row').length).toBe(2);
 
             // Check for the correct default in the new row
-            const $environmentInput = $wrapper.find('.js-row:last-child').find('input[name="variables[variables_attributes][][environment]"]');
+            const $environmentInput = $wrapper.find('.js-row:last-child').find('input[name="variables[variables_attributes][][environment_scope]"]');
             expect($environmentInput.val()).toBe('*');
           })
           .then(done)
           .catch(done.fail);
       });
+
+      it('should update dropdown with new environment values and remove values when row is removed', (done) => {
+        addRowByNewEnvironment('someenv');
+
+        const $row = $wrapper.find('.js-row:last-child');
+        $row.find('.js-variable-environment-toggle').click();
+
+        getSetTimeoutPromise()
+          .then(() => {
+            const $dropdownItemsBeforeRemove = $row.find('.js-variable-environment-dropdown-wrapper .dropdown-content a');
+            expect($dropdownItemsBeforeRemove.length).toBe(2);
+            expect($dropdownItemsBeforeRemove[0].textContent.trim()).toBe('someenv');
+            expect($dropdownItemsBeforeRemove[1].textContent.trim()).toBe('* (All environments)');
+
+            $wrapper.find('.js-row-remove-button').trigger('click');
+
+            expect($wrapper.find('.js-row').length).toBe(0);
+          })
+          .then(() => {
+            const $dropdownItemsAfterRemove = $row.find('.js-variable-environment-dropdown-wrapper .dropdown-content a');
+            expect($dropdownItemsAfterRemove.length).toBe(1);
+            expect($dropdownItemsAfterRemove[0].textContent.trim()).toBe('* (All environments)');
+          })
+          .then(done)
+          .catch(done.fail);
+      });
     });
   });
 });

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);
+    });
+  });
 });

spec/javascripts/fixtures/groups.rb

+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

spec/javascripts/fixtures/projects.rb

 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,10 @@ describe ProjectsController, '(JavaScript fixtures)', type: :controller do
   end
 
   before do
+    # EE-specific start
+    stub_licensed_features(variable_environment_scope: true)
+    # EE specific end
+    project.add_master(admin)
     sign_in(admin)
   end
 
@@ -21,6 +28,7 @@ describe ProjectsController, '(JavaScript fixtures)', type: :controller do
     remove_repository(project)
   end
 
+  describe ProjectsController, '(JavaScript fixtures)', type: :controller do
     it 'projects/dashboard.html.raw' do |example|
       get :show,
         namespace_id: project.namespace.to_param,
@@ -38,4 +46,25 @@ describe ProjectsController, '(JavaScript fixtures)', type: :controller do
       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

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 }
 
-      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

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 }
 
-      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

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)

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)

spec/serializers/group_variable_entity_spec.rb

+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

spec/serializers/variable_entity_spec.rb

+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

spec/support/features/variable_list_shared_examples.rb

+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

spec/support/shared_examples/controllers/variables_shared_examples.rb

+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