diff --git a/lib/friendly_id/history.rb b/lib/friendly_id/history.rb index a26a22c2..1f483f75 100644 --- a/lib/friendly_id/history.rb +++ b/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) diff --git a/test/candidates_test.rb b/test/candidates_test.rb index b280e365..c7898aea 100644 --- a/test/candidates_test.rb +++ b/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 diff --git a/test/history_test.rb b/test/history_test.rb index f9c64434..b8801741 100644 --- a/test/history_test.rb +++ b/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