From 3ea04616c38493ae03b31f54c88b6551b6d65b6f Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 26 Jun 2017 02:59:27 +0900 Subject: [PATCH] Implement variables_attributes create/update cases --- .../projects/pipeline_schedules_controller.rb | 5 +- app/models/ci/pipeline_schedule.rb | 10 - .../ci/create_pipeline_schedule_service.rb | 30 +- .../pipeline_schedules_controller_spec.rb | 355 +++++++++++++++++- 4 files changed, 377 insertions(+), 23 deletions(-) diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb index fdbf930a5ef..2ee6229cf68 100644 --- a/app/controllers/projects/pipeline_schedules_controller.rb +++ b/app/controllers/projects/pipeline_schedules_controller.rb @@ -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 diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb index 61215b1fb46..06b2b981252 100644 --- a/app/models/ci/pipeline_schedule.rb +++ b/app/models/ci/pipeline_schedule.rb @@ -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 diff --git a/app/services/ci/create_pipeline_schedule_service.rb b/app/services/ci/create_pipeline_schedule_service.rb index cd40deb6187..4c9cb5c66c6 100644 --- a/app/services/ci/create_pipeline_schedule_service.rb +++ b/app/services/ci/create_pipeline_schedule_service.rb @@ -1,13 +1,39 @@ 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 diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index a8c44d5c313..106d4b7496b 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -36,20 +36,357 @@ describe Projects::PipelineSchedulesController do end end - describe 'GET edit' do - let(:user) { create(:user) } - + describe 'GET #new' do before do - project.add_master(user) - - sign_in(user) + create(:user).tap do |user| + project.add_developer(user) + sign_in(user) + end end - it 'loads the pipeline schedule' do - get :edit, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id + it 'initializes a pipeline schedule model' do + get :new, namespace_id: project.namespace.to_param, project_id: project expect(response).to have_http_status(:ok) - expect(assigns(:schedule)).to eq(pipeline_schedule) + expect(assigns(:schedule)).to be_a_new(Ci::PipelineSchedule) + end + end + + describe 'POST #create' do + 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) do + basic_param + end + + it 'creates a new schedule' do + 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) + + expect(response).to have_http_status(:found) + end + end + + context 'when variables_attributes has one variable' do + let(:schedule) do + basic_param.merge({ + variables_attributes: [ { key: 'AAA', value: 'AAA123' } ] + }) + end + + it 'creates a new schedule' do + 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) + + expect(response).to have_http_status(:found) + expect(Ci::PipelineScheduleVariable.last.key).to eq("AAA") + expect(Ci::PipelineScheduleVariable.last.value).to eq("AAA123") + end + + context 'when the same key has already been persisted' do + it 'returns an error that the key of variable is invaild' do + 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 + end + end + end + + context 'when variables_attributes has one variable and key is empty' do + let(:schedule) do + basic_param.merge({ + variables_attributes: [ { key: '', value: 'AAA123' } ] + }) + end + + it 'returns an error that the key of variable is invaild' do + 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) + + expect(assigns(:schedule).errors['variables.key']).not_to be_empty + end + end + + 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' } ] + }) + end + + it 'creates a new schedule' do + 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) + + expect(response).to have_http_status(:found) + expect(Ci::PipelineScheduleVariable.first.key).to eq("AAA") + expect(Ci::PipelineScheduleVariable.first.value).to eq("AAA123") + expect(Ci::PipelineScheduleVariable.last.key).to eq("BBB") + expect(Ci::PipelineScheduleVariable.last.value).to eq("BBB123") + end + end + + 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' } ] + }) + end + + it 'returns an error that the keys of variable are duplicated' do + 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) + + expect(assigns(:schedule).errors['variables.key']).not_to be_empty + end + end + end + + describe 'PUT #update' do + before do + create(:user).tap do |user| + project.add_developer(user) + sign_in(user) + end + end + + 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 + 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 * * *') + expect(pipeline_schedule.cron_timezone).to eq('UTC') + expect(pipeline_schedule.ref).to eq('patch-x') + expect(pipeline_schedule.active).to eq(true) + expect(pipeline_schedule.variables).to be_empty + end + end + + context 'when params include one variable' do + let(:schedule) do + basic_param.merge({ + variables_attributes: [ { key: 'AAA', value: 'AAA123' } ] + }) + end + + it 'inserts new variable to the pipeline schedule' do + 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') + end + end + + context 'when params include two unique variables' do + let(:schedule) do + basic_param.merge({ + variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'BBB', value: 'BBB123' } ] + }) + end + + it 'inserts two new variables to the pipeline schedule' do + 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') + expect(pipeline_schedule.variables.last.key).to eq('BBB') + expect(pipeline_schedule.variables.last.value).to eq('BBB123') + end + end + + context 'when params include two duplicated variables' do + let(:schedule) do + basic_param.merge({ + variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' } ] + }) + end + + it 'returns an error that variables are duplciated' do + put :update, namespace_id: project.namespace.to_param, + project_id: project, id: pipeline_schedule, schedule: schedule + + expect(assigns(:schedule).errors['variables.key']).not_to be_empty + end + end + end + + context 'when a pipeline schedule has one variable' do + let!(:pipeline_schedule_variable) do + 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 + 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 * * *') + expect(pipeline_schedule.cron_timezone).to eq('UTC') + expect(pipeline_schedule.ref).to eq('patch-x') + expect(pipeline_schedule.active).to eq(true) + expect(pipeline_schedule.variables.count).to eq(1) + expect(pipeline_schedule.variables.last.key).to eq('CCC') + end + end + + context 'when params include one variable' do + context 'when adds a new variable' do + let(:schedule) do + basic_param.merge({ + variables_attributes: [ { key: 'AAA', value: 'AAA123' }] + }) + end + + it 'adds the new variable' do + 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) + + expect(pipeline_schedule.variables.last.key).to eq('AAA') + end + end + + context 'when updates a variable' do + let(:schedule) do + basic_param.merge({ + variables_attributes: [ { id: pipeline_schedule_variable.id, value: 'new_value' } ] + }) + end + + it 'updates the variable' do + 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 + + context 'when deletes a variable' do + let(:schedule) do + basic_param.merge({ + variables_attributes: [ { id: pipeline_schedule_variable.id, _destroy: true } ] + }) + end + + it 'delete the existsed variable' do + 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 'GET edit' do + context 'TODO: integrate to bottom' do + let(:user) { create(:user) } + + 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 + context 'when the developer edits' do + it 'can edit variables' do + # TODO: + end + end + + context 'when other developers edit' do + it 'can not edit variables' do + # TODO: + end + end + + context 'when a master edits' do + it 'can edit variables' do + # TODO: + end + end + end + + context 'when a master created a pipeline schedule' do + context 'when the master edits' do + it 'can edit variables' do + # TODO: + end + end + + context 'when other masters edit' do + it 'can edit variables' do + # TODO: + end + end + + context 'when developers edit' do + it 'can not edit variables' do + # TODO: + end + end end end