Fix collision errors when introducing history plugin after the fact

lib/friendly_id/history.rb

@@ -102,23 +102,17 @@ def slug_history_clause(id)
 
     private
 
-    # If we're updating, don't consider historic slugs for the same record
-    # to be conflicts. This will allow a record to revert to a previously
-    # used slug.
     def scope_for_slug_generator
-      relation = super.joins(:slugs)
-      unless new_record?
-        relation = relation.merge(Slug.where("sluggable_id <> ?", id))
-      end
-      if friendly_id_config.uses?(:scoped)
-        relation = relation.where(Slug.arel_table[:scope].eq(serialized_scope))
-      end
-      relation
+      relation = super.left_joins(:slugs)
+      return relation unless friendly_id_config.uses?(:scoped)
+
+      relation.where(Slug.arel_table[:scope].eq(serialized_scope))
     end
 
     def create_slug
       return unless friendly_id
       return if history_is_up_to_date?
+
       # Allow reversion back to a previously used slug
       relation = slugs.where(slug: friendly_id)
       if friendly_id_config.uses?(:scoped)

test/candidates_test.rb

@@ -139,4 +139,28 @@ def slug_candidates
       assert_equal collected_candidates, ["new-york"]
     end
   end
+
+  test "should not fail when adding history to existing" do
+    name_collision = "Amsterdam"
+
+    with_instances do |city1, _|
+      assert city1.update name: name_collision, slug: nil
+      assert_equal name_collision.downcase, city1.slug
+
+      klass = Class.new city1.class do
+        friendly_id_config.model_class = city1.class
+        friendly_id_config.use(:history)
+
+        def slug_candidates
+          [:name, [:name, "-alt"]]
+        end
+      end
+      assert klass.friendly_id_config.uses? :history
+
+      city2 = klass.last
+
+      assert city2.update name: name_collision, slug: nil
+      assert_equal "#{name_collision.downcase}-alt", city2.slug
+    end
+  end
 end

test/history_test.rb

@@ -83,12 +83,15 @@ def model_class
       record.name = "x"
       record.slug = nil
       assert record.save
+      assert record.slug == "x"
       record.name = "y"
       record.slug = nil
       assert record.save
+      assert record.slug == "y"
       record.name = "x"
       record.slug = nil
       assert record.save
+      assert record.slug == "x"
     end
   end
 
@@ -114,6 +117,20 @@ def model_class
     end
   end
 
+  test "should not get an old slug used by another" do
+    transaction do
+      record = model_class.create! :name => "Old Name"
+      assert_equal record.slug, "old-name"
+      record.update :name => "New Name", :slug => nil
+      assert_equal record.slug, "new-name"
+
+      new_record = model_class.create! :name => "Old Name"
+      assert_match(/old-name(-\w+){5}/, new_record.slug)
+
+      assert_equal model_class.friendly.find("old-name"), record
+    end
+  end
+
   test "should not create new slugs that match old slugs" do
     transaction do
       first_record = model_class.create! name: "foo"
@@ -432,3 +449,50 @@ def model_class
     end
   end
 end
+
+class MigrationTest < TestCaseClass
+  include FriendlyId::Test
+
+  class City < ActiveRecord::Base
+    extend FriendlyId
+    friendly_id :slug_candidates, use: :slugged
+    alias_attribute :slug_candidates, :name
+  end
+
+  def with_migrate_scenario(city_name = "New York", &block)
+    transaction do
+      city = City.create! name: city_name
+
+      klass = Class.new City do
+        friendly_id_config.model_class = City
+        friendly_id_config.use(:history)
+
+        def slug_candidates
+          [:name, [:name, "-alt"]]
+        end
+      end
+
+      yield city, klass
+    end
+  end
+
+  test "should not fail when adding history to existing" do
+    name_collision = "Amsterdam"
+
+    with_migrate_scenario(name_collision) do |city, klass|
+      city2 = klass.create! name: "New York"
+
+      assert city2.update name: name_collision, slug: nil
+      assert_equal "#{name_collision.downcase}-alt", city2.slug
+    end
+  end
+
+  test "scope_for_slug_generator should find slugs not in Slug table" do
+    with_migrate_scenario do |city, klass|
+      assert klass.new.send(:scope_for_slug_generator).include? city
+
+      city2 = klass.create! name: city.name
+      assert city2.send(:scope_for_slug_generator).include? city
+    end
+  end
+end