Don't check uniqueness if !permalink_changed?

Bug fix: check uniqueness when assigned using permalink=
Increase test coverage for uniqueness and update checking.

Author: martinemde

README

@@ -20,4 +20,18 @@ These are set to nil if Iconv is not loaded.  You can also manually set them to
 	end
 
 Use the :if or :unless options to specify a Proc, method, or string to be called or evaluated. The permalink
-will only be generated if the option evaluates to true.
+will only be generated if the option evaluates to true.
+
+[Added 3.11.2009 by Martin Emde] Make permalink_fu update your permalink everytime the dependent field(s) change.
+
+	class Article < ActiveRecord::Base
+	  has_permalink :title, :update => true
+	end
+
+Without :update set to true, your permalink will be set one time and subsequent changes to the field
+(title in this example) will not affect the permalink field. To regenerate the permalink field,
+set it to nil or a blank string within your model.
+
+Old versions of rails without _changed? attribute support will result in the permalink field being regenerated every save.
+
+[Fixed 3.11.2009 by Martin Emde] Permalink was not being checked for uniqueness when set directly with permalink=

lib/permalink_fu.rb

@@ -54,7 +54,7 @@ module PluginMethods
     #     has_permalink :title, :unique => false
     #
     #     # update the permalink every time the attribute(s) change
-    #     # (relies on _changed? methods to function effectively)
+    #     # without _changed? methods (old rails version) this will rewrite the permalink every time
     #     has_permalink :title, :update => true
     #
     #   end
@@ -109,20 +109,23 @@ module InstanceMethods
   protected
     def create_common_permalink
       return unless should_create_permalink?
-      if read_attribute(self.class.permalink_field).blank? || self.class.permalink_options[:update]
+      if read_attribute(self.class.permalink_field).blank? || permalink_fields_changed?
         send("#{self.class.permalink_field}=", create_permalink_for(self.class.permalink_attributes))
       end
-      if read_attribute(self.class.permalink_field).blank?
-        send("#{self.class.permalink_field}=", PermalinkFu.random_permalink)
-      end
+
+      # Quit now if we have the changed method available and nothing has changed
+      permalink_changed = "#{self.class.permalink_field}_changed?"
+      return if respond_to?(permalink_changed) && !send(permalink_changed)
+
+      # Otherwise find the limit and crop the permalink
       limit   = self.class.columns_hash[self.class.permalink_field].limit
       base    = send("#{self.class.permalink_field}=", read_attribute(self.class.permalink_field)[0..limit - 1])
       [limit, base]
     end
 
     def create_unique_permalink
       limit, base = create_common_permalink
-      return if limit.nil?
+      return if limit.nil? # nil if the permalink has not changed or :if/:unless fail
       counter = 1
       # oh how i wish i could use a hash for conditions
       conditions = ["#{self.class.permalink_field} = ?", base]
@@ -149,13 +152,12 @@ def create_unique_permalink
     end
 
     def create_permalink_for(attr_names)
-      attr_names.collect { |attr_name| send(attr_name).to_s } * " "
+      str = attr_names.collect { |attr_name| send(attr_name).to_s } * " "
+      str.blank? ? PermalinkFu.random_permalink : str
     end
 
   private
     def should_create_permalink?
-      existing_value = send("#{self.class.permalink_field}")
-      return false unless permalink_fields_changed? || existing_value.blank?
       if self.class.permalink_options[:if]
         evaluate_method(self.class.permalink_options[:if])
       elsif self.class.permalink_options[:unless]
@@ -165,7 +167,9 @@ def should_create_permalink?
       end
     end
 
+    # Don't even check _changed? methods unless :update is set
     def permalink_fields_changed?
+      return false unless self.class.permalink_options[:update]
       self.class.permalink_attributes.any? do |attribute|
         changed_method = "#{attribute}_changed?"
         respond_to?(changed_method) ? send(changed_method) : true

test/permalink_fu_test.rb

@@ -111,6 +111,26 @@ def self.exists?(conditions)
   has_permalink :title
 end
 
+class PermalinkChangeableMockModel < BaseModel
+  def self.exists?(conditions)
+    if conditions[1] == 'foo'
+      true
+    else
+      false
+    end
+  end
+
+  has_permalink :title
+
+  def permalink_changed?
+    @permalink_changed
+  end
+
+  def permalink_will_change!
+    @permalink_changed = true
+  end
+end
+
 class CommonMockModel < BaseModel
   def self.exists?(conditions)
     false # oh noes
@@ -147,21 +167,21 @@ def permalink
   end
 end
 
-class ChangedModel < BaseModel
+class ChangedWithoutUpdateModel < BaseModel
   has_permalink :title  
   def title_changed?; true; end
 end
 
-class NoChangeModel < BaseModel
-  has_permalink :title  
-  def title_changed?; false; end
-end
-
 class ChangedWithUpdateModel < BaseModel
   has_permalink :title, :update => true 
   def title_changed?; true; end
 end
 
+class NoChangeModel < BaseModel
+  has_permalink :title, :update => true
+  def title_changed?; false; end
+end
+
 class IfProcConditionModel < BaseModel
   has_permalink :title, :if => Proc.new { |obj| false }
 end
@@ -239,13 +259,26 @@ def test_multiple_attribute_permalink
       end
     end
   end
-  
+
   def test_should_create_unique_permalink
+    @m = MockModel.new
+    @m.title = 'foo'
+    @m.validate
+    assert_equal 'foo-2', @m.permalink
+    
+    @m.title = 'bar'
+    @m.permalink = nil
+    @m.validate
+    assert_equal 'bar-3', @m.permalink
+  end
+  
+  def test_should_create_unique_permalink_when_assigned_directly
     @m = MockModel.new
     @m.permalink = 'foo'
     @m.validate
     assert_equal 'foo-2', @m.permalink
 
+    # should always check itself for uniqueness when not respond_to?(:permalink_changed?)
     @m.permalink = 'bar'
     @m.validate
     assert_equal 'bar-3', @m.permalink
@@ -265,6 +298,21 @@ def test_should_not_check_itself_for_unique_permalink
     @m.validate
     assert_equal 'bar-2', @m.permalink
   end
+
+  def test_should_check_itself_for_unique_permalink_if_permalink_field_changed
+    @m = PermalinkChangeableMockModel.new
+    @m.permalink_will_change!
+    @m.permalink = 'foo'
+    @m.validate
+    assert_equal 'foo-2', @m.permalink
+  end
+
+  def test_should_not_check_itself_for_unique_permalink_if_permalink_field_not_changed
+    @m = PermalinkChangeableMockModel.new
+    @m.permalink = 'foo'
+    @m.validate
+    assert_equal 'foo', @m.permalink
+  end
 
   def test_should_create_unique_scoped_permalink
     @m = ScopedModel.new
@@ -361,11 +409,12 @@ def test_should_not_update_permalink_unless_field_changed
     assert_equal 'unchanged', @m.read_attribute(:permalink)
   end
 
-  def test_should_update_permalink_if_field_changed
-    @m = OverrideModel.new
+  def test_should_not_update_permalink_without_update_set_even_if_field_changed
+    @m = ChangedWithoutUpdateModel.new
     @m.title = 'the permalink'
+    @m.permalink = 'unchanged'
     @m.validate
-    assert_equal 'the-permalink', @m.read_attribute(:permalink)
+    assert_equal 'unchanged', @m.read_attribute(:permalink)
   end
 
   def test_should_update_permalink_if_changed_method_does_not_exist
@@ -391,7 +440,7 @@ def test_should_update_permalink_if_the_existing_permalink_is_blank
     assert_equal 'the-permalink', @m.read_attribute(:permalink)
   end
 
-  def test_should_update_permalink_if_the_title_is_nil
+  def test_should_assign_a_random_permalink_if_the_title_is_nil
     @m = NoChangeModel.new
     @m.title = nil
     @m.validate
@@ -400,7 +449,7 @@ def test_should_update_permalink_if_the_title_is_nil
   end
 
   def test_should_update_permalink_the_first_time_the_title_is_set
-    @m = ChangedModel.new
+    @m = ChangedWithoutUpdateModel.new
     @m.title = "old title"
     @m.validate
     assert_equal "old-title", @m.read_attribute(:permalink)
@@ -410,8 +459,8 @@ def test_should_update_permalink_the_first_time_the_title_is_set
   end
 
   def test_should_not_update_permalink_if_already_set_even_if_title_changed
-    @m = ChangedModel.new
-    @m.permalink = "old-permalink"
+    @m = ChangedWithoutUpdateModel.new
+    @m.permalink = "old permalink"
     @m.title = "new title"
     @m.validate
     assert_equal "old-permalink", @m.read_attribute(:permalink)