diff options
| -rw-r--r-- | app/models/ci/pipeline_schedule.rb | 5 | ||||
| -rw-r--r-- | app/validators/uniqueness_of_in_memory_validator.rb | 37 | ||||
| -rw-r--r-- | spec/controllers/projects/pipeline_schedules_controller_spec.rb | 67 |
3 files changed, 72 insertions, 37 deletions
diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb index df9df45edb0..ad9f8b89924 100644 --- a/app/models/ci/pipeline_schedule.rb +++ b/app/models/ci/pipeline_schedule.rb @@ -15,6 +15,11 @@ module Ci validates :cron_timezone, cron_timezone: true, presence: { unless: :importing? } validates :ref, presence: { unless: :importing? } validates :description, presence: true + validates :variables, uniqueness_of_in_memory: { + :collection => :variables, + :attrs => [:pipeline_schedule_id, :key], + :message => ['variables.key', 'keys are duplicated'] + } before_save :set_next_run_at diff --git a/app/validators/uniqueness_of_in_memory_validator.rb b/app/validators/uniqueness_of_in_memory_validator.rb new file mode 100644 index 00000000000..84e88b2eb76 --- /dev/null +++ b/app/validators/uniqueness_of_in_memory_validator.rb @@ -0,0 +1,37 @@ +# UniquenessOfInMemoryValidator +# +# This validtor is designed for especially the following condition +# - Use `accepts_nested_attributes_for :xxx` in a parent model +# - Use `validates :xxx, uniqueness: { scope: :xxx_id }` in a child model +# +# Inspired by https://stackoverflow.com/a/2883129/2522666 +module ActiveRecord + class Base + # Validate that the the objects in +collection+ are unique + # when compared against all their non-blank +attrs+. If not + # add +message+ to the base errors. + def validate_uniqueness_of_in_memory(collection, attrs, message) + hashes = collection.inject({}) do |hash, record| + key = attrs.map { |a| record.send(a).to_s }.join + if key.blank? || record.marked_for_destruction? + key = record.object_id + end + hash[key] = record unless hash[key] + hash + end + + if collection.length > hashes.length + self.errors.add(*message) + end + end + end +end + +class UniquenessOfInMemoryValidator < ActiveModel::Validator + def validate(record) + record.validate_uniqueness_of_in_memory( + record.public_send(options[:collection]), + options[:attrs], + options[:message]) + end +end diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index a3a5fe32307..12eeca5408b 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -150,22 +150,21 @@ describe Projects::PipelineSchedulesController do 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 { go } - # .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 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 { go } + .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 @@ -194,7 +193,6 @@ describe Projects::PipelineSchedulesController do before do project.add_developer(user) - sign_in(user) end @@ -210,7 +208,6 @@ describe Projects::PipelineSchedulesController do go 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 * * *') @@ -232,7 +229,6 @@ describe Projects::PipelineSchedulesController do expect { go }.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') @@ -250,7 +246,6 @@ describe Projects::PipelineSchedulesController do expect { go }.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') @@ -259,21 +254,20 @@ describe Projects::PipelineSchedulesController do 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 + 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 @@ -293,7 +287,6 @@ describe Projects::PipelineSchedulesController do go 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 * * *') @@ -316,6 +309,7 @@ describe Projects::PipelineSchedulesController do it 'adds the new variable' do expect { go }.to change { Ci::PipelineScheduleVariable.count }.by(1) + pipeline_schedule.reload expect(pipeline_schedule.variables.last.key).to eq('AAA') end end @@ -331,7 +325,6 @@ describe Projects::PipelineSchedulesController do expect { go }.not_to change { Ci::PipelineScheduleVariable.count } pipeline_schedule_variable.reload - expect(pipeline_schedule_variable.value).to eq('new_value') end end |