summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/models/ci/pipeline_schedule.rb5
-rw-r--r--app/validators/uniqueness_of_in_memory_validator.rb37
-rw-r--r--spec/controllers/projects/pipeline_schedules_controller_spec.rb67
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