From bd488c04483ce35d1a5d8359f3bafc360a630083 Mon Sep 17 00:00:00 2001
From: James Adam <james@lazyatom.com>
Date: Thu, 17 May 2018 11:09:56 +0100
Subject: [PATCH 1/2] Add tests to demonstrate value of to_param

These tests were prompted by the presence of lots of deprecation
warnings under Rails 5.1, due to the calls to the `attribute_changed?`
method within `to_param`, whose behaviour was expected to change in
Rails 5.2. I believe the fix might involve a significant change to
`to_param`, so I've also add other tests to comprehensively describe
the behaviour of `to_param` in various scenarios where the record
succeeded or failed to save.

I believe the new callback test also demonstrates a bug in all
versions of FriendlyId up to Rails 5.2; the value of the slug used in
the callback is the old value. This is a problem if we use that slug
to generate a URL as part of behaviour called within that callback.
---
 test/slugged_test.rb | 80 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 79 insertions(+), 1 deletion(-)

diff --git a/test/slugged_test.rb b/test/slugged_test.rb
index 4a4943d14..388864119 100644
--- a/test/slugged_test.rb
+++ b/test/slugged_test.rb
@@ -152,7 +152,6 @@ def self.name
       assert_equal 'foo', instance.slug
     end
   end
-
 end
 
 class SlugGeneratorTest < TestCaseClass
@@ -419,6 +418,85 @@ class Journalist < ActiveRecord::Base
 
 end
 
+class ToParamTest < TestCaseClass
+
+  include FriendlyId::Test
+
+  class Journalist < ActiveRecord::Base
+    extend FriendlyId
+    validates_presence_of :active
+    friendly_id :name, :use => :slugged
+
+    attr_accessor :to_param_in_callback
+
+    after_save do
+      self.to_param_in_callback = to_param
+    end
+  end
+
+  test "to_param should return nil if record is unpersisted" do
+    assert_nil Journalist.new.to_param
+  end
+
+  test "to_param should return nil if record failed validation" do
+    journalist = Journalist.new :name => 'Clark Kent', :active => nil
+    refute journalist.save
+    assert_nil journalist.to_param
+  end
+
+  test "to_param should use slugged attribute if record saved successfully" do
+    transaction do
+      journalist = Journalist.new :name => 'Clark Kent', :active => true
+      assert journalist.save
+      assert_equal 'clark-kent', journalist.to_param
+    end
+  end
+
+  test "to_param should use original slug if existing record changes but fails to save" do
+    transaction do
+      journalist = Journalist.new :name => 'Clark Kent', :active => true
+      assert journalist.save
+      journalist.name = 'Superman'
+      journalist.slug = nil
+      journalist.active = nil
+      refute journalist.save
+      assert_equal 'clark-kent', journalist.to_param
+    end
+  end
+
+  test "to_param should use new slug if existing record changes successfully" do
+    transaction do
+      journalist = Journalist.new :name => 'Clark Kent', :active => true
+      assert journalist.save
+      journalist.name = 'Superman'
+      journalist.slug = nil
+      assert journalist.save
+      assert_equal 'superman', journalist.to_param
+    end
+  end
+
+  test "to_param should use new slug within callbacks if new record is saved successfully" do
+    transaction do
+      journalist = Journalist.new :name => 'Clark Kent', :active => true
+      assert journalist.save
+      assert_equal 'clark-kent', journalist.to_param_in_callback, "value of to_param in callback should use the new slug value"
+    end
+  end
+
+  test "to_param should use new slug within callbacks if existing record changes successfully" do
+    transaction do
+      journalist = Journalist.new :name => 'Clark Kent', :active => true
+      assert journalist.save
+      assert journalist.valid?
+      journalist.name = 'Superman'
+      journalist.slug = nil
+      assert journalist.save, "save should be successful"
+      assert_equal 'superman', journalist.to_param_in_callback, "value of to_param in callback should use the new slug value"
+    end
+  end
+
+end
+
 class ConfigurableRoutesTest < TestCaseClass
   include FriendlyId::Test
 

From 3df38192be9f53f740527bc3305fac67124a6854 Mon Sep 17 00:00:00 2001
From: James Adam <james@lazyatom.com>
Date: Thu, 17 May 2018 14:56:15 +0100
Subject: [PATCH 2/2] Fix behaviour of to_param in callbacks

This ensures that we use the new value of a slug when calling
`to_param` within an ActiveRecord callback. It also seems to pass all
the other regression tests I've added, thus keeping the fixes working
for #259 and #148, and as a bonus, prevents getting a bunch of
deprecation warnings in Rails 5.1.
---
 lib/friendly_id/base.rb | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/lib/friendly_id/base.rb b/lib/friendly_id/base.rb
index 2844d1498..7ab56e032 100644
--- a/lib/friendly_id/base.rb
+++ b/lib/friendly_id/base.rb
@@ -261,12 +261,7 @@ def friendly_id
     # Either the friendly_id, or the numeric id cast to a string.
     def to_param
       if friendly_id_config.routes == :friendly
-        if attribute_changed?(friendly_id_config.query_field)
-          diff = changes[friendly_id_config.query_field]
-          diff.first || diff.second
-        else
-          friendly_id.presence.to_param || super
-        end
+        friendly_id.presence.to_param || super
       else
         super
       end