From 49801f2f4c6a3bd121fd861091ef494215713837 Mon Sep 17 00:00:00 2001
From: Joe Rafaniello <jrafanie@redhat.com>
Date: Thu, 7 Jun 2018 16:37:19 -0400
Subject: [PATCH] Allow duplicate nil pxe_image_types during seed

https://bugzilla.redhat.com/show_bug.cgi?id=1588417

We want to disallow the same name template in different regions with the
same pxe_image_type.  Normally, region id sequences prevent this from
happening but we were treating nil pxe_image_types as the same.
Therefore you could have the "same" pxe_image_type in different regions
for the same name template.
---
 app/models/customization_template.rb       |  7 +++-
 spec/models/customization_template_spec.rb | 46 ++++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100644 spec/models/customization_template_spec.rb

diff --git a/app/models/customization_template.rb b/app/models/customization_template.rb
index 5b3d43db8eca..c90583248cc8 100644
--- a/app/models/customization_template.rb
+++ b/app/models/customization_template.rb
@@ -5,11 +5,16 @@ class CustomizationTemplate < ApplicationRecord
   has_many   :pxe_images, :through => :pxe_image_type
 
   validates :pxe_image_type, :presence => true, :unless => :system?
-  validates :name,           :unique_within_region => true
+  validates :name, :unique_within_region => true
+  validates :name, :uniqueness => {:scope => :pxe_image_type } unless :system_and_no_pxe_image_type?
 
   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) }
 
+  def system_and_no_pxe_image_type?
+    system? && pxe_image_type_id.nil?
+  end
+
   def self.seed_file_name
     @seed_file_name ||= Rails.root.join("db", "fixtures", "#{table_name}.yml")
   end
diff --git a/spec/models/customization_template_spec.rb b/spec/models/customization_template_spec.rb
new file mode 100644
index 000000000000..7eef86bb5e8e
--- /dev/null
+++ b/spec/models/customization_template_spec.rb
@@ -0,0 +1,46 @@
+describe CustomizationTemplate do
+  context "unique name validation" do
+    let(:pxe_image_type) { FactoryGirl.create(:pxe_image_type) }
+
+    it "doesn't allow same name in same region" do
+      FactoryGirl.create(:customization_template, :name => "fred")
+      expect { FactoryGirl.create(:customization_template, :name => "fred") }.to raise_error(ActiveRecord::RecordInvalid)
+    end
+
+    it "allows different names with the same pxe_image_type in same region" do
+      FactoryGirl.create(:customization_template, :name => "fred", :pxe_image_type => pxe_image_type)
+      FactoryGirl.create(:customization_template, :name => "nick", :pxe_image_type => pxe_image_type)
+      expect(described_class.count).to eq(2)
+
+      first, second = described_class.all
+      expect(first.name).to_not eq(second.name)
+      expect(first.pxe_image_type).to eq(second.pxe_image_type)
+      expect(ApplicationRecord.id_to_region(first.id)).to eq(ApplicationRecord.id_to_region(second.id))
+    end
+
+    it "allow same name, nil pxe_image_types in different regions (for system seeding)" do
+      system_template   = described_class.create!(:system => true, :name => "nick")
+      other_template_id = ApplicationRecord.id_in_region(system_template.id, ApplicationRecord.my_region_number + 1)
+      described_class.create!(:system => true, :name => "nick", :id => other_template_id)
+
+      first, second = described_class.all
+      expect(first.name).to eq(second.name)
+      expect(first.pxe_image_type).to be nil
+      expect(first.pxe_image_type).to eq(second.pxe_image_type)
+      expect(ApplicationRecord.id_to_region(first.id)).to_not eq(ApplicationRecord.id_to_region(second.id))
+    end
+
+    it "allows same name, different pxe_image_types in different regions" do
+      template          = FactoryGirl.create(:customization_template, :name => "nick", :pxe_image_type => pxe_image_type)
+      other_template_id = ApplicationRecord.id_in_region(template.id, MiqRegion.my_region_number + 1)
+      other_pit_id      = ApplicationRecord.id_in_region(pxe_image_type.id, MiqRegion.my_region_number + 1)
+      other_pit         = FactoryGirl.create(:pxe_image_type, :id => other_pit_id)
+      FactoryGirl.create(:customization_template, :name => "nick", :pxe_image_type => other_pit, :id => other_template_id )
+
+      first, second = described_class.all
+      expect(first.name).to eq(second.name)
+      expect(first.pxe_image_type).to_not eq(second.pxe_image_type)
+      expect(ApplicationRecord.id_to_region(first.id)).to_not eq(ApplicationRecord.id_to_region(second.id))
+    end
+  end
+end