Skip to content

G
gitlab-ce
Commit e364c118 authored by Shinya Maeda's avatar Shinya Maeda
Browse files
Options

Implement variables_attributes create/update cases

parent 507fedf3
Show whitespace changes
Inline Side-by-side
Showing with 315 additions and 298 deletions
app/controllers/projects/pipeline_schedules_controller.rb
View file @ e364c118
......@@ -33,7 +33,8 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
end
def update
if schedule.update(schedule_params)
if Ci::CreatePipelineScheduleService
.new(@project, current_user, schedule_params).update(schedule)
redirect_to namespace_project_pipeline_schedules_path(@project.namespace.becomes(Namespace), @project)
else
render :edit
......@@ -67,6 +68,6 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
def schedule_params
params.require(:schedule)
.permit(:description, :cron, :cron_timezone, :ref, :active,
variables_attributes: [:key, :value] )
variables_attributes: [:id, :key, :value, :_destroy] )
end
end
app/models/ci/pipeline_schedule.rb
View file @ e364c118
......@@ -15,7 +15,6 @@ module Ci
validates :cron_timezone, cron_timezone: true, presence: { unless: :importing? }
validates :ref, presence: { unless: :importing? }
validates :description, presence: true
validates_associated :variables
before_save :set_next_run_at
......@@ -24,15 +23,6 @@ module Ci
accepts_nested_attributes_for :variables, allow_destroy: true
before_validation(on: :update) do
# TODO: if validation failed, restore the deleted_obj
deleted_obj = Ci::PipelineScheduleVariable.where(pipeline_schedule_id: self).destroy_all
end
after_validation(on: :update) do
# TODO: if validation failed, restore the deleted_obj
end
def owned_by?(current_user)
owner == current_user
end
......
app/services/ci/create_pipeline_schedule_service.rb
View file @ e364c118
module Ci
class CreatePipelineScheduleService < BaseService
def execute
project.pipeline_schedules.create(pipeline_schedule_params)
pipeline_schedule = project.pipeline_schedules.build(pipeline_schedule_params)
if variable_keys_duplicated?
pipeline_schedule.errors.add('variables.key', "keys are duplicated")
return pipeline_schedule
end
pipeline_schedule.save
pipeline_schedule
end
def update(pipeline_schedule)
if variable_keys_duplicated?
pipeline_schedule.errors.add('variables.key', "keys are duplicated")
return false
end
pipeline_schedule.update(pipeline_schedule_params)
end
private
def pipeline_schedule_params
params.merge(owner: current_user)
@pipeline_schedule_params ||= params.merge(owner: current_user)
end
def variable_keys_duplicated?
attributes = pipeline_schedule_params['variables_attributes']
return false unless attributes.is_a?(Array)
attributes.map { |v| v['key'] }.uniq.length != attributes.length
end
end
end
spec/controllers/projects/pipeline_schedules_controller_spec.rb
View file @ e364c118
require 'spec_helper'
describe Projects::PipelineSchedulesController do
include AccessMatchersForController
set(:project) { create(:empty_project, :public) }
let!(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project) }
......@@ -19,14 +17,6 @@ describe Projects::PipelineSchedulesController do
expect(response).to render_template(:index)
end
it 'avoids N + 1 queries' do
control_count = ActiveRecord::QueryRecorder.new { visit_pipelines_schedules }.count
create_list(:ci_pipeline_schedule, 2, project: project)
expect { visit_pipelines_schedules }.not_to exceed_query_limit(control_count)
end
context 'when the scope is set to active' do
let(:scope) { 'active' }
......@@ -47,12 +37,12 @@ describe Projects::PipelineSchedulesController do
end
describe 'GET #new' do
set(:user) { create(:user) }
before do
create(:user).tap do |user|
project.add_developer(user)
sign_in(user)
end
end
it 'initializes a pipeline schedule model' do
get :new, namespace_id: project.namespace.to_param, project_id: project
......@@ -63,23 +53,24 @@ describe Projects::PipelineSchedulesController do
end
describe 'POST #create' do
describe 'functionality' do
set(:user) { create(:user) }
before do
create(:user).tap do |user|
project.add_developer(user)
sign_in(user)
end
end
let(:basic_param) do
{ description: 'aaaaaaaa', cron: '0 4 * * *', cron_timezone: 'UTC', ref: 'master', active: '1' }
end
context 'when variables_attributes is empty' do
let(:schedule) { basic_param }
let(:schedule) do
basic_param
end
it 'creates a new schedule' do
expect { go }
expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule }
.to change { Ci::PipelineSchedule.count }.by(1)
.and change { Ci::PipelineScheduleVariable.count }.by(0)
......@@ -90,12 +81,12 @@ describe Projects::PipelineSchedulesController do
context 'when variables_attributes has one variable' do
let(:schedule) do
basic_param.merge({
variables_attributes: [{ key: 'AAA', value: 'AAA123' }]
variables_attributes: [ { key: 'AAA', value: 'AAA123' } ]
})
end
it 'creates a new schedule' do
expect { go }
expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule }
.to change { Ci::PipelineSchedule.count }.by(1)
.and change { Ci::PipelineScheduleVariable.count }.by(1)
......@@ -106,7 +97,7 @@ describe Projects::PipelineSchedulesController do
context 'when the same key has already been persisted' do
it 'returns an error that the key of variable is invaild' do
go
post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule
pipeline_schedule_variable = build(:ci_pipeline_schedule_variable, key: 'AAA', pipeline_schedule: assigns(:schedule))
expect(pipeline_schedule_variable).to be_invalid
......@@ -117,12 +108,12 @@ describe Projects::PipelineSchedulesController do
context 'when variables_attributes has one variable and key is empty' do
let(:schedule) do
basic_param.merge({
variables_attributes: [{ key: '', value: 'AAA123' }]
variables_attributes: [ { key: '', value: 'AAA123' } ]
})
end
it 'returns an error that the key of variable is invaild' do
expect { go }
expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule }
.to change { Ci::PipelineSchedule.count }.by(0)
.and change { Ci::PipelineScheduleVariable.count }.by(0)
......@@ -133,12 +124,12 @@ describe Projects::PipelineSchedulesController do
context 'when variables_attributes has two variables and unique' do
let(:schedule) do
basic_param.merge({
variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'BBB', value: 'BBB123' }]
variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'BBB', value: 'BBB123' } ]
})
end
it 'creates a new schedule' do
expect { go }
expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule }
.to change { Ci::PipelineSchedule.count }.by(1)
.and change { Ci::PipelineScheduleVariable.count }.by(2)
......@@ -153,12 +144,12 @@ describe Projects::PipelineSchedulesController do
context 'when variables_attributes has two variables and duplicted' do
let(:schedule) do
basic_param.merge({
variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' }]
variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' } ]
})
end
it 'returns an error that the keys of variable are duplicated' do
expect { go }
expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule }
.to change { Ci::PipelineSchedule.count }.by(0)
.and change { Ci::PipelineScheduleVariable.count }.by(0)
......@@ -167,47 +158,28 @@ describe Projects::PipelineSchedulesController do
end
end
describe 'security' do
let(:schedule) { { description: 'aaaaaaaa', cron: '0 4 * * *', cron_timezone: 'UTC', ref: 'master', active: '1' } }
it { expect { go }.to be_allowed_for(:admin) }
it { expect { go }.to be_allowed_for(:owner).of(project) }
it { expect { go }.to be_allowed_for(:master).of(project) }
it { expect { go }.to be_allowed_for(:developer).of(project) }
it { expect { go }.to be_denied_for(:reporter).of(project) }
it { expect { go }.to be_denied_for(:guest).of(project) }
it { expect { go }.to be_denied_for(:user) }
it { expect { go }.to be_denied_for(:external) }
it { expect { go }.to be_denied_for(:visitor) }
end
def go
post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule
end
end
describe 'PUT #update' do
describe 'functionality' do
set(:user) { create(:user) }
let!(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project, owner: user) }
before do
create(:user).tap do |user|
project.add_developer(user)
sign_in(user)
end
end
context 'when a pipeline schedule has no variables' do
let(:basic_param) do
{ description: 'updated_desc', cron: '0 1 * * *', cron_timezone: 'UTC', ref: 'patch-x', active: '1' }
end
context 'when a pipeline schedule has no variables' do
context 'when params do not include variables' do
let(:schedule) { basic_param }
it 'updates only scheduled pipeline attributes' do
go
put :update, namespace_id: project.namespace.to_param,
project_id: project, id: pipeline_schedule, schedule: schedule
pipeline_schedule.reload
expect(response).to have_http_status(:found)
expect(pipeline_schedule.description).to eq('updated_desc')
expect(pipeline_schedule.cron).to eq('0 1 * * *')
......@@ -221,14 +193,18 @@ describe Projects::PipelineSchedulesController do
context 'when params include one variable' do
let(:schedule) do
basic_param.merge({
variables_attributes: [{ key: 'AAA', value: 'AAA123' }]
variables_attributes: [ { key: 'AAA', value: 'AAA123' } ]
})
end
it 'inserts new variable to the pipeline schedule' do
expect { go }.to change { Ci::PipelineScheduleVariable.count }.by(1)
expect do
put :update, namespace_id: project.namespace.to_param,
project_id: project, id: pipeline_schedule, schedule: schedule
end.to change { Ci::PipelineScheduleVariable.count }.by(1)
pipeline_schedule.reload
expect(response).to have_http_status(:found)
expect(pipeline_schedule.variables.last.key).to eq('AAA')
expect(pipeline_schedule.variables.last.value).to eq('AAA123')
......@@ -238,14 +214,18 @@ describe Projects::PipelineSchedulesController do
context 'when params include two unique variables' do
let(:schedule) do
basic_param.merge({
variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'BBB', value: 'BBB123' }]
variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'BBB', value: 'BBB123' } ]
})
end
it 'inserts two new variables to the pipeline schedule' do
expect { go }.to change { Ci::PipelineScheduleVariable.count }.by(2)
expect do
put :update, namespace_id: project.namespace.to_param,
project_id: project, id: pipeline_schedule, schedule: schedule
end.to change { Ci::PipelineScheduleVariable.count }.by(2)
pipeline_schedule.reload
expect(response).to have_http_status(:found)
expect(pipeline_schedule.variables.first.key).to eq('AAA')
expect(pipeline_schedule.variables.first.value).to eq('AAA123')
......@@ -257,7 +237,7 @@ describe Projects::PipelineSchedulesController do
context 'when params include two duplicated variables' do
let(:schedule) do
basic_param.merge({
variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' }]
variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' } ]
})
end
......@@ -271,22 +251,20 @@ describe Projects::PipelineSchedulesController do
end
context 'when a pipeline schedule has one variable' do
let(:basic_param) do
{ description: 'updated_desc', cron: '0 1 * * *', cron_timezone: 'UTC', ref: 'patch-x', active: '1' }
end
let!(:pipeline_schedule_variable) do
create(:ci_pipeline_schedule_variable,
key: 'CCC', pipeline_schedule: pipeline_schedule)
create(:ci_pipeline_schedule_variable, key: 'CCC',
pipeline_schedule: pipeline_schedule)
end
context 'when params do not include variables' do
let(:schedule) { basic_param }
it 'updates only scheduled pipeline attributes' do
go
put :update, namespace_id: project.namespace.to_param,
project_id: project, id: pipeline_schedule, schedule: schedule
pipeline_schedule.reload
expect(response).to have_http_status(:found)
expect(pipeline_schedule.description).to eq('updated_desc')
expect(pipeline_schedule.cron).to eq('0 1 * * *')
......@@ -302,14 +280,16 @@ describe Projects::PipelineSchedulesController do
context 'when adds a new variable' do
let(:schedule) do
basic_param.merge({
variables_attributes: [{ key: 'AAA', value: 'AAA123' }]
variables_attributes: [ { key: 'AAA', value: 'AAA123' }]
})
end
it 'adds the new variable' do
expect { go }.to change { Ci::PipelineScheduleVariable.count }.by(1)
expect do
put :update, namespace_id: project.namespace.to_param,
project_id: project, id: pipeline_schedule, schedule: schedule
end.to change { Ci::PipelineScheduleVariable.count }.by(1)
pipeline_schedule.reload
expect(pipeline_schedule.variables.last.key).to eq('AAA')
end
end
......@@ -317,14 +297,18 @@ describe Projects::PipelineSchedulesController do
context 'when updates a variable' do
let(:schedule) do
basic_param.merge({
variables_attributes: [{ id: pipeline_schedule_variable.id, value: 'new_value' }]
variables_attributes: [ { id: pipeline_schedule_variable.id, value: 'new_value' } ]
})
end
it 'updates the variable' do
expect { go }.not_to change { Ci::PipelineScheduleVariable.count }
expect do
put :update, namespace_id: project.namespace.to_param,
project_id: project, id: pipeline_schedule, schedule: schedule
end.not_to change { Ci::PipelineScheduleVariable.count }
pipeline_schedule_variable.reload
expect(pipeline_schedule_variable.value).to eq('new_value')
end
end
......@@ -332,145 +316,161 @@ describe Projects::PipelineSchedulesController do
context 'when deletes a variable' do
let(:schedule) do
basic_param.merge({
variables_attributes: [{ id: pipeline_schedule_variable.id, _destroy: true }]
variables_attributes: [ { id: pipeline_schedule_variable.id, _destroy: true } ]
})
end
it 'delete the existsed variable' do
expect { go }.to change { Ci::PipelineScheduleVariable.count }.by(-1)
expect do
put :update, namespace_id: project.namespace.to_param,
project_id: project, id: pipeline_schedule, schedule: schedule
end.to change { Ci::PipelineScheduleVariable.count }.by(-1)
end
end
end
end
end
describe 'security' do
let(:schedule) { { description: 'updated_desc' } }
describe 'GET edit' do
context 'TODO: integrate to bottom' do
let(:user) { create(:user) }
it { expect { go }.to be_allowed_for(:admin) }
it { expect { go }.to be_allowed_for(:owner).of(project) }
it { expect { go }.to be_allowed_for(:master).of(project) }
it { expect { go }.to be_allowed_for(:developer).of(project).own([pipeline_schedule]) }
it { expect { go }.to be_denied_for(:reporter).of(project) }
it { expect { go }.to be_denied_for(:guest).of(project) }
it { expect { go }.to be_denied_for(:user) }
it { expect { go }.to be_denied_for(:external) }
it { expect { go }.to be_denied_for(:visitor) }
before do
project.add_master(user)
sign_in(user)
end
it 'loads the pipeline schedule' do
get :edit, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
expect(response).to have_http_status(:ok)
expect(assigns(:schedule)).to eq(pipeline_schedule)
end
end
context 'when a developer created a pipeline schedule' do
let(:developer_1) { create(:user) }
let!(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project, owner: developer_1) }
context 'when the developer edits' do
it 'can edit variables' do
# TODO:
end
end
before do
project.add_developer(developer_1)
context 'when other developers edit' do
it 'can not edit variables' do
# TODO:
end
end
it { expect { go }.to be_allowed_for(developer_1) }
it { expect { go }.to be_denied_for(:developer).of(project) }
it { expect { go }.to be_allowed_for(:master).of(project) }
context 'when a master edits' do
it 'can edit variables' do
# TODO:
end
end
end
context 'when a master created a pipeline schedule' do
let(:master_1) { create(:user) }
let!(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project, owner: master_1) }
before do
project.add_master(master_1)
context 'when the master edits' do
it 'can edit variables' do
# TODO:
end
end
it { expect { go }.to be_allowed_for(master_1) }
it { expect { go }.to be_allowed_for(:master).of(project) }
it { expect { go }.to be_denied_for(:developer).of(project) }
context 'when other masters edit' do
it 'can edit variables' do
# TODO:
end
end
def go
put :update, namespace_id: project.namespace.to_param,
project_id: project, id: pipeline_schedule,
schedule: schedule
context 'when developers edit' do
it 'can not edit variables' do
# TODO:
end
end
end
end
describe 'GET #edit' do
describe 'functionality' do
let(:user) { create(:user) }
describe 'DELETE #destroy' do
set(:user) { create(:user) }
context 'when a developer makes the request' do
before do
project.add_developer(user)
sign_in(user)
delete :destroy, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
end
it 'does not delete the pipeline schedule' do
expect(response).not_to have_http_status(:ok)
end
end
context 'when a master makes the request' do
before do
project.add_master(user)
sign_in(user)
end
it 'loads the pipeline schedule' do
get :edit, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
it 'destroys the pipeline schedule' do
expect do
delete :destroy, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
end.to change { project.pipeline_schedules.count }.by(-1)
expect(response).to have_http_status(:ok)
expect(assigns(:schedule)).to eq(pipeline_schedule)
expect(response).to have_http_status(302)
end
end
end
describe 'security' do
include AccessMatchersForController
describe 'GET edit' do
it { expect { go }.to be_allowed_for(:admin) }
it { expect { go }.to be_allowed_for(:owner).of(project) }
it { expect { go }.to be_allowed_for(:master).of(project) }
it { expect { go }.to be_allowed_for(:developer).of(project).own([pipeline_schedule]) }
it { expect { go }.to be_allowed_for(:developer).of(project) }
it { expect { go }.to be_denied_for(:reporter).of(project) }
it { expect { go }.to be_denied_for(:guest).of(project) }
it { expect { go }.to be_denied_for(:user) }
it { expect { go }.to be_denied_for(:external) }
it { expect { go }.to be_denied_for(:visitor) }
end
def go
get :edit, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
end
end
describe 'GET #take_ownership' do
describe 'security' do
describe 'GET take_ownership' do
it { expect { go }.to be_allowed_for(:admin) }
it { expect { go }.to be_allowed_for(:owner).of(project) }
it { expect { go }.to be_allowed_for(:master).of(project) }
it { expect { go }.to be_allowed_for(:developer).of(project).own([pipeline_schedule]) }
it { expect { go }.to be_allowed_for(:developer).of(project) }
it { expect { go }.to be_denied_for(:reporter).of(project) }
it { expect { go }.to be_denied_for(:guest).of(project) }
it { expect { go }.to be_denied_for(:user) }
it { expect { go }.to be_denied_for(:external) }
it { expect { go }.to be_denied_for(:visitor) }
end
def go
post :take_ownership, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
end
end
describe 'DELETE #destroy' do
set(:user) { create(:user) }
context 'when a developer makes the request' do
before do
project.add_developer(user)
sign_in(user)
delete :destroy, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
end
it 'does not delete the pipeline schedule' do
expect(response).not_to have_http_status(:ok)
end
end
context 'when a master makes the request' do
before do
project.add_master(user)
sign_in(user)
end
it 'destroys the pipeline schedule' do
expect do
delete :destroy, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id
end.to change { project.pipeline_schedules.count }.by(-1)
describe 'PUT update' do
it { expect { go }.to be_allowed_for(:admin) }
it { expect { go }.to be_allowed_for(:owner).of(project) }
it { expect { go }.to be_allowed_for(:master).of(project) }
it { expect { go }.to be_allowed_for(:developer).of(project) }
it { expect { go }.to be_denied_for(:reporter).of(project) }
it { expect { go }.to be_denied_for(:guest).of(project) }
it { expect { go }.to be_denied_for(:user) }
it { expect { go }.to be_denied_for(:external) }
it { expect { go }.to be_denied_for(:visitor) }
expect(response).to have_http_status(302)
def go
put :update, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id,
schedule: { description: 'a' }
end
end
end
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7