From dc58520907a588f412b2aaf534ddc43f41daca79 Mon Sep 17 00:00:00 2001 From: Maarten Jacobs Date: Fri, 14 Apr 2017 16:55:11 +0200 Subject: [PATCH 1/4] introduce a test that replays upgrading the model with the history module --- test/candidates_test.rb | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) 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 From d26a325b220f571f6d88b80f5909e9dcbd22c612 Mon Sep 17 00:00:00 2001 From: Maarten Jacobs Date: Tue, 18 Apr 2017 21:45:54 +0200 Subject: [PATCH 2/4] do not (inner!) join the slug table when calculating scope for slug_generator -> it should be irrelevant --- lib/friendly_id/history.rb | 2 +- test/history_test.rb | 61 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/lib/friendly_id/history.rb b/lib/friendly_id/history.rb index a26a22c2..fe459ec7 100644 --- a/lib/friendly_id/history.rb +++ b/lib/friendly_id/history.rb @@ -106,7 +106,7 @@ def slug_history_clause(id) # to be conflicts. This will allow a record to revert to a previously # used slug. def scope_for_slug_generator - relation = super.joins(:slugs) + relation = super.includes(:slugs) unless new_record? relation = relation.merge(Slug.where("sluggable_id <> ?", id)) end diff --git a/test/history_test.rb b/test/history_test.rb index f9c64434..e1a7737d 100644 --- a/test/history_test.rb +++ b/test/history_test.rb @@ -114,6 +114,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 +446,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 From b09e508704b1ad5862a820859f4c22447023f5a6 Mon Sep 17 00:00:00 2001 From: Maarten Jacobs Date: Wed, 30 Mar 2022 15:34:28 +0200 Subject: [PATCH 3/4] remove superfluous code -- renaming back to a previous slug was possible without the extra merged where --- lib/friendly_id/history.rb | 14 +++++--------- test/history_test.rb | 3 +++ 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/friendly_id/history.rb b/lib/friendly_id/history.rb index fe459ec7..1626f034 100644 --- a/lib/friendly_id/history.rb +++ b/lib/friendly_id/history.rb @@ -102,18 +102,14 @@ 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.includes(:slugs) - unless new_record? - relation = relation.merge(Slug.where("sluggable_id <> ?", id)) - end + relation = super.left_joins(:slugs) + if friendly_id_config.uses?(:scoped) - relation = relation.where(Slug.arel_table[:scope].eq(serialized_scope)) + relation.where(Slug.arel_table[:scope].eq(serialized_scope)) + else + relation end - relation end def create_slug diff --git a/test/history_test.rb b/test/history_test.rb index e1a7737d..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 From 446aae16d2e45e0dd174a9a4ffce2e06a2ec3de8 Mon Sep 17 00:00:00 2001 From: Philip Arndt Date: Fri, 22 Jul 2022 11:39:39 +1200 Subject: [PATCH 4/4] Use guard clause --- lib/friendly_id/history.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/friendly_id/history.rb b/lib/friendly_id/history.rb index 1626f034..1f483f75 100644 --- a/lib/friendly_id/history.rb +++ b/lib/friendly_id/history.rb @@ -104,17 +104,15 @@ def slug_history_clause(id) def scope_for_slug_generator relation = super.left_joins(:slugs) - - if friendly_id_config.uses?(:scoped) - relation.where(Slug.arel_table[:scope].eq(serialized_scope)) - else - relation - end + 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)