diff options
3 files changed, 34 insertions, 59 deletions
diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb index 86c9628d917..9bcdfc2b510 100644 --- a/app/controllers/projects/pipeline_schedules_controller.rb +++ b/app/controllers/projects/pipeline_schedules_controller.rb @@ -33,8 +33,7 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController end def update - if Ci::CreatePipelineScheduleService - .new(@project, current_user, schedule_params).update(schedule) + if schedule.update(schedule_params) redirect_to namespace_project_pipeline_schedules_path(@project.namespace.becomes(Namespace), @project) else render :edit diff --git a/app/services/ci/create_pipeline_schedule_service.rb b/app/services/ci/create_pipeline_schedule_service.rb index 4c9cb5c66c6..cd40deb6187 100644 --- a/app/services/ci/create_pipeline_schedule_service.rb +++ b/app/services/ci/create_pipeline_schedule_service.rb @@ -1,39 +1,13 @@ module Ci class CreatePipelineScheduleService < BaseService def execute - 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) + project.pipeline_schedules.create(pipeline_schedule_params) end private def pipeline_schedule_params - @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 + params.merge(owner: current_user) end end end diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index 5f6b3c0a187..1567bde2a62 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -142,21 +142,22 @@ describe Projects::PipelineSchedulesController do 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 + # This test no longer passes, since we removed a custom validation + # 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 'security' do @@ -260,20 +261,21 @@ describe Projects::PipelineSchedulesController do 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 + # This test no longer passes, since we removed a custom validation + # 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 |