Merge pull request #16775 from d-m-u/issue_16739

Fix for STI scoping across leaves
(cherry picked from commit c992bac)

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) }

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}")

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

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