favor after_save callbacks, its easier to force tasks to run before or after after_save callbacks

3 files changed, 32 insertions, 27 deletions

diff --git a/lib/acts_as_versioned.rb b/lib/acts_as_versioned.rb
index b036c75da..7aa72b2d6 100644
--- a/lib/acts_as_versioned.rb
+++ b/lib/acts_as_versioned.rb
@@ -66,7 +66,7 @@ module ActiveRecord #:nodoc:
     #
     # See ActiveRecord::Acts::Versioned::ClassMethods#acts_as_versioned for configuration options
     module Versioned
-      CALLBACKS = [:set_new_version, :save_version_on_create, :save_version?]
+      CALLBACKS = [:set_new_version, :save_version, :save_version?]
       def self.included(base) # :nodoc:
         base.extend ClassMethods
       end
@@ -214,8 +214,7 @@ module ActiveRecord #:nodoc:
               end
             end
             before_save  :set_new_version
-            after_create :save_version_on_create
-            after_update :save_version
+            after_save   :save_version
             after_save   :clear_old_versions
 
             unless options[:if_changed].nil?
@@ -271,18 +270,16 @@ module ActiveRecord #:nodoc:
           base.extend ClassMethods
         end
 
-        # Saves a version of the model if applicable
-        def save_version
-          save_version_on_create if save_version?
-        end
-
         # Saves a version of the model in the versioned table.  This is called in the after_save callback by default
-        def save_version_on_create
-          rev = self.class.versioned_class.new
-          clone_versioned_model(self, rev)
-          rev.version = send(self.class.version_column)
-          rev.send("#{self.class.versioned_foreign_key}=", id)
-          rev.save
+        def save_version
+          if @is_new_record || save_version?
+            @is_new_record = nil
+            rev = self.class.versioned_class.new
+            clone_versioned_model(self, rev)
+            rev.version = send(self.class.version_column)
+            rev.send("#{self.class.versioned_foreign_key}=", id)
+            rev.save
+          end
         end
 
         # Clears old revisions if a limit is set with the :limit option in <tt>acts_as_versioned</tt>.
@@ -389,6 +386,7 @@ module ActiveRecord #:nodoc:
         protected
           # sets the new version before saving, unless you're using optimistic locking.  In that case, let it take care of the version.
           def set_new_version
+            @is_new_record = new_record?
             self.send("#{self.class.version_column}=", next_version) if new_record? || (!locking_enabled? && save_version?)
           end
 
diff --git a/test/schema.rb b/test/schema.rb
index 5d2938b49..06051a406 100644
--- a/test/schema.rb
+++ b/test/schema.rb
@@ -18,6 +18,8 @@ ActiveRecord::Schema.define(:version => 0) do
     t.column :revisor_id, :integer
   end
   
+  add_index :page_versions, [:page_id, :version], :unique => true
+  
   create_table :authors, :force => true do |t|
     t.column :page_id, :integer
     t.column :name, :string
@@ -36,6 +38,8 @@ ActiveRecord::Schema.define(:version => 0) do
     t.column :version_type, :string, :limit => 255
     t.column :updated_at, :datetime
   end
+  
+  add_index :locked_pages_revisions, [:page_id, :version], :unique => true
 
   create_table :widgets, :force => true do |t|
     t.column :name, :string, :limit => 50
@@ -51,6 +55,8 @@ ActiveRecord::Schema.define(:version => 0) do
     t.column :updated_at, :datetime
   end
   
+  add_index :widget_versions, [:widget_id, :version], :unique => true
+  
   create_table :landmarks, :force => true do |t|
     t.column :name, :string
     t.column :latitude, :float
@@ -67,4 +73,6 @@ ActiveRecord::Schema.define(:version => 0) do
     t.column :doesnt_trigger_version,:string
     t.column :version, :integer
   end
+  
+  add_index :landmark_versions, [:landmark_id, :version], :unique => true
 end
diff --git a/test/versioned_test.rb b/test/versioned_test.rb
index 3331ec4be..624553086 100644
--- a/test/versioned_test.rb
+++ b/test/versioned_test.rb
@@ -147,15 +147,15 @@ class VersionedTest < Test::Unit::TestCase
 
     p = Page.create! :title => "title"
     assert_equal 1, p.version # version does not increment
-    assert_equal 1, p.versions(true).size
+    assert_equal 1, p.versions.count
 
     p.update_attributes(:title => 'new title')
     assert_equal 1, p.version # version does not increment
-    assert_equal 1, p.versions(true).size
+    assert_equal 1, p.versions.count
 
     p.update_attributes(:title => 'a title')
     assert_equal 2, p.version
-    assert_equal 2, p.versions(true).size
+    assert_equal 2, p.versions.count
 
     # reset original if condition
     Page.class_eval { alias_method :feeling_good?, :old_feeling_good }
@@ -168,15 +168,15 @@ class VersionedTest < Test::Unit::TestCase
 
     p = Page.create! :title => "title"
     assert_equal 1, p.version # version does not increment
-    assert_equal 1, p.versions(true).size
+    assert_equal 1, p.versions.count
 
     p.update_attributes(:title => 'a title')
     assert_equal 1, p.version # version does not increment
-    assert_equal 1, p.versions(true).size
+    assert_equal 1, p.versions.count
 
     p.update_attributes(:title => 'b title')
     assert_equal 2, p.version
-    assert_equal 2, p.versions(true).size
+    assert_equal 2, p.versions.count
 
     # reset original if condition
     Page.version_condition = old_condition
@@ -322,25 +322,24 @@ class VersionedTest < Test::Unit::TestCase
   def test_should_find_version_count
     assert_equal 2, pages(:welcome).versions.size
   end
-  
+
   def test_if_changed_creates_version_if_a_listed_column_is_changed
-    landmarks(:washington).name="Washington"
+    landmarks(:washington).name = "Washington"
     assert landmarks(:washington).changed?
     assert landmarks(:washington).altered?
   end
 
   def test_if_changed_creates_version_if_all_listed_columns_are_changed
-    landmarks(:washington).name="Washington"
-    landmarks(:washington).latitude=1.0
-    landmarks(:washington).longitude=1.0
+    landmarks(:washington).name = "Washington"
+    landmarks(:washington).latitude = 1.0
+    landmarks(:washington).longitude = 1.0
     assert landmarks(:washington).changed?
     assert landmarks(:washington).altered?
   end
 
   def test_if_changed_does_not_create_new_version_if_unlisted_column_is_changed
-    landmarks(:washington).doesnt_trigger_version="This should not trigger version"
+    landmarks(:washington).doesnt_trigger_version = "This should not trigger version"
     assert landmarks(:washington).changed?
     assert !landmarks(:washington).altered?
   end
-
 end
\ No newline at end of file