From ad0f581fde579a157d6ff01b9aca89e21b5b216e Mon Sep 17 00:00:00 2001 From: d-m-u Date: Tue, 9 Jan 2018 09:56:24 -0500 Subject: [PATCH] Fix for STI scoping across leafs --- app/models/customization_template.rb | 2 +- lib/unique_within_region_validator.rb | 5 +- spec/factories/pxe_image_type.rb | 1 + .../unique_within_region_validator_spec.rb | 123 ++++++++++-------- 4 files changed, 76 insertions(+), 55 deletions(-) diff --git a/app/models/customization_template.rb b/app/models/customization_template.rb index c6bf2ab8f89..f152bb1be76 100644 --- a/app/models/customization_template.rb +++ b/app/models/customization_template.rb @@ -5,7 +5,7 @@ class CustomizationTemplate < ApplicationRecord has_many :pxe_images, :through => :pxe_image_type validates :pxe_image_type, :presence => true, :unless => :system? - validates :name, :uniqueness => { :scope => :pxe_image_type }, :unique_within_region => true + validates :name, :unique_within_region => true scope :with_pxe_image_type_id, ->(pxe_image_type_id) { where(:pxe_image_type_id => pxe_image_type_id) } scope :with_system, ->(bool = true) { where(:system => bool) } diff --git a/lib/unique_within_region_validator.rb b/lib/unique_within_region_validator.rb index a8a8dda3af0..2837ae05078 100644 --- a/lib/unique_within_region_validator.rb +++ b/lib/unique_within_region_validator.rb @@ -2,11 +2,12 @@ class UniqueWithinRegionValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) return if value.nil? match_case = options.key?(:match_case) ? options[:match_case] : true + record_base_class = record.class.base_class field_matches = if match_case - record.class.where(attribute => value) + record_base_class.where(attribute => value) else - record.class.where(record.class.arel_attribute(attribute).lower.eq(value.downcase)) + record_base_class.where(record_base_class.arel_attribute(attribute).lower.eq(value.downcase)) end unless field_matches.in_region(record.region_id).where.not(:id => record.id).empty? record.errors.add(attribute, "is not unique within region #{record.region_id}") diff --git a/spec/factories/pxe_image_type.rb b/spec/factories/pxe_image_type.rb index c23c4843c79..4f81d594523 100644 --- a/spec/factories/pxe_image_type.rb +++ b/spec/factories/pxe_image_type.rb @@ -1,4 +1,5 @@ FactoryGirl.define do factory :pxe_image_type do + sequence(:name) { |n| "pxe_image_type_#{seq_padded_for_sorting(n)}" } end end diff --git a/spec/lib/unique_within_region_validator_spec.rb b/spec/lib/unique_within_region_validator_spec.rb index f70fba4e51c..b492acf82d5 100644 --- a/spec/lib/unique_within_region_validator_spec.rb +++ b/spec/lib/unique_within_region_validator_spec.rb @@ -1,73 +1,92 @@ describe UniqueWithinRegionValidator do describe "#unique_within_region" do - let(:case_sensitive_class) do - Class.new(User).tap do |c| - c.class_eval do - validates :name, :unique_within_region => true + context "class without STI" do + let(:case_sensitive_class) do + Class.new(User).tap do |c| + c.class_eval do + validates :name, :unique_within_region => true + end end end - end - let(:case_insensitive_class) do - Class.new(User).tap do |c| - c.class_eval do - validates :name, :unique_within_region => {:match_case => false} + let(:case_insensitive_class) do + Class.new(User).tap do |c| + c.class_eval do + validates :name, :unique_within_region => {:match_case => false} + end end end - end - let(:test_name) { "thename" } + let(:test_name) { "thename" } - let(:in_first_region_id) do - FactoryGirl.create( - :user, - :id => case_sensitive_class.id_in_region(1, 0), - :name => test_name - ).id - end + let(:in_first_region_id) do + FactoryGirl.create( + :user, + :id => case_sensitive_class.id_in_region(1, 0), + :name => test_name + ).id + end - let(:also_in_first_region_id) do - FactoryGirl.create( - :user, - :id => case_sensitive_class.id_in_region(2, 0), - :name => test_name.upcase - ).id - end + let(:also_in_first_region_id) do + FactoryGirl.create( + :user, + :id => case_sensitive_class.id_in_region(2, 0), + :name => test_name.upcase + ).id + end - let(:in_second_region_id) do - FactoryGirl.create( - :user, - :id => case_sensitive_class.id_in_region(2, 1), - :name => test_name - ).id - end + let(:in_second_region_id) do + FactoryGirl.create( + :user, + :id => case_sensitive_class.id_in_region(2, 1), + :name => test_name + ).id + end - it "returns true if the field is unique for the records in the region" do - expect(case_sensitive_class.find(in_first_region_id).valid?).to be true - expect(case_sensitive_class.find(also_in_first_region_id).valid?).to be true - expect(case_sensitive_class.find(in_second_region_id).valid?).to be true - end + it "returns true if the field is unique for the records in the region" do + expect(case_sensitive_class.find(in_first_region_id).valid?).to be true + expect(case_sensitive_class.find(also_in_first_region_id).valid?).to be true + expect(case_sensitive_class.find(in_second_region_id).valid?).to be true + end - it "returns false if the field is not unique for the records in the region" do - in_first_region_rec = case_sensitive_class.find(in_first_region_id) - also_in_first_region_rec = case_sensitive_class.find(also_in_first_region_id) - in_second_region_rec = case_sensitive_class.find(in_second_region_id) + it "returns false if the field is not unique for the records in the region" do + in_first_region_rec = case_sensitive_class.find(in_first_region_id) + also_in_first_region_rec = case_sensitive_class.find(also_in_first_region_id) + in_second_region_rec = case_sensitive_class.find(in_second_region_id) - also_in_first_region_rec.name = in_first_region_rec.name + also_in_first_region_rec.name = in_first_region_rec.name - expect(in_first_region_rec.valid?).to be true - expect(also_in_first_region_rec.valid?).to be false - expect(in_second_region_rec.valid?).to be true + expect(in_first_region_rec.valid?).to be true + expect(also_in_first_region_rec.valid?).to be false + expect(in_second_region_rec.valid?).to be true + end + + it "is case insensitive if match_case is set to false" do + in_first_region_rec = case_insensitive_class.find(in_first_region_id) + also_in_first_region_rec = case_insensitive_class.find(also_in_first_region_id) + in_second_region_rec = case_insensitive_class.find(in_second_region_id) + + expect(in_first_region_rec.valid?).to be false + expect(also_in_first_region_rec.valid?).to be false + expect(in_second_region_rec.valid?).to be true + end end - it "is case insensitive if match_case is set to false" do - in_first_region_rec = case_insensitive_class.find(in_first_region_id) - also_in_first_region_rec = case_insensitive_class.find(also_in_first_region_id) - in_second_region_rec = case_insensitive_class.find(in_second_region_id) + context "class with STI" do + context "two subclasses" do + it "raises error with non-unique names in same region" do + FactoryGirl.create(:customization_template_cloud_init, :name => "foo") - expect(in_first_region_rec.valid?).to be false - expect(also_in_first_region_rec.valid?).to be false - expect(in_second_region_rec.valid?).to be true + expect { FactoryGirl.create(:customization_template_sysprep, :name => "foo") } + .to raise_error(ActiveRecord::RecordInvalid, / Name is not unique within region/) + end + + it "doesn't raise error with unique names" do + FactoryGirl.create(:customization_template_cloud_init) + + expect { FactoryGirl.create(:customization_template_sysprep) }.to_not raise_error + end + end end end end