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

Dynamic CI secret variables -- CE backport

See merge request gitlab-org/gitlab-ce!16842

20 files changed, 827 insertions, 355 deletions

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